Shopify / packwerk

Good things come in small packages.
MIT License
1.54k stars 111 forks source link

[Bug Report] has_many relationship for serializers create false positive violations #370

Closed huan-ji closed 2 months ago

huan-ji commented 10 months ago

Description Currently packwerk assumes that all has_many relationships are relationships to models (ActiveRecord for example). Such as example below:

class Example
  has_many :accounts
end

Running above code on packwerk assumes that the relationship is to an Account model. However this assumption seems to also apply when the has_many relationship is written for a serializer:

class ExampleSerializer < ActiveModel::Serializer
  has_many :accounts
end

packwerk would also assume the above code's has_many relationship is to an Account model, when it should be to the AccountSerializer model. Even if the serializer is explicitly defined, there will still be a violation to the Account model based on the has_many relationship.

This issue seems to apply to any serializers that has the has_many relationship syntax to define relationships to other serializers, here are a few examples: https://github.com/rails-api/active_model_serializers https://github.com/yosiat/panko_serializer https://github.com/okuramasafumi/alba https://github.com/cerebris/jsonapi-resources

To Reproduce Write a serializer using active_model_serializers with a has_many relationship and put the serializer into a pack, run packwerk check on it and you should see a dependency violation on a model instead of the related serializer. See this demo project for an example of this https://github.com/huan-ji/has_many_demo

See here for the post serializer, this serializer has a has_many relationship which should point to the CommentSerializer, both of these serializers live in the pack so there shouldn't be any violations, but as you can see here in the package_todo file, there is a violation on the Comment model.

Expected Behaviour When running packwerk check for a serializer file that contains a has_many relationship, there should not be a violation for the model instead of the serializer, the code should not assume all has_many relationships are to models.

Screenshots image

Version Information

gmcgibbon commented 9 months ago

Hm, perhaps this method is generic and we need to look at the class it is defined on. I'll investigate!