Class Name Collision with Packwerk: How to Avoid and Potential Solutions

Tools Packwerk
1

This message was imported from the Ruby/Rails Modularity Slack server. Find more info in the import thread.

EDIT: packwerk validate detects the issue explained below :point_down:

Hi :wave:, we recently faced what would probably be the first cons of using Packwerk in our monolith. It is class name collision.

Without packwerk (assuming you follow the Rails naming conventions) it’s hard to have a class name collision, because (at least for us) there is a single file where the class will be defined.

With packwerk now there are N places (one per package + [root]/app/**/*) where the same class can be defined, because packwerk doesn’t enforce namespaces for their packages. One way to avoid this issue would be to make sure to always use the package name as the namespace, which sounds reasonable, but at the same time that would make migrating to packwerk packages a bit harder, since you’ll need to refactor the entire codebase where the classes are referenced.

Also, IIRC in Stephan’s book he brings up the idea to add the ability to enforce namespaces on components (such as engines behave). I think this would also help prevent mistakes (sometimes it can be confusing to see packages/my_package/app/models/my_class.rb define MyClass instead of MyPackage::MyClass )

Any thoughts on this? Have you experienced similar situations? Something you want to share? :smile:

2

Message originally sent by slack user U783MOJYF8Z

Great analysis of the problem IMO.

Packwerk was extracted from tooling for our huge monolith, where putting everything into package specific namespaces was not a realistic option. Indeed, packwerk’s analysis engine could be greatly simplified if we could assume that top level namespace == package name.

I know Gusto is building stuff on top / around that does more opinionated enforcement.

However, I’m curious. Can you give more details on the collision you’re seeing? The most obvious cases should be prevented by an existing check in constant_resolver that makes sure two files don’t have the same full path (relative to the autoload paths).

3

Oh nice! I didn’t know there was a check in packwerk :trophy: that’d help since we run packwerk validate and check commands on the CI.

Now that you mention this, maybe it didn’t get to happen, but what did happen was that a teammate told me that he wanted to create a class that would have caused this. Basically, imagine we have

packages
├── component_a
│   └── app
└── component_b
    └── app

he wanted to create a class in component_b that referred to component_a , so he wanted to use component_a as a namespace under component_b like

packages
├── component_a
│   └── app
└── component_b
    └── app
        └── models
            └── component_a
                └── my_class.rb

which would have been a problem because we already had the same class name in component_a respecting the appropriate namespace in this case, like this

packages
├── component_a
│   └── app
|       └── models
|           └── component_a
|               └── my_class.rb
└── component_b
    └── app
        └── models
            └── component_a
                └── my_class.rb

But I’ll test if packwerk would have caught this and let you know :100:

4 
📦 Packwerk is running validation...

Validation failed ❗
ERROR: 'Iam::Group' could refer to any of
  app/models/iam/group.rb
  components/iam/app/models/iam/group.rb
  components/iam_admin/app/models/iam/group.rb

Beautiful! This makes me feel so much safer now :relaxed: Thanks Phillip

And yes, I completely understand that it wouldn’t have been realistic for Shopify to enforce namespaces :+1: BTW I was reading https://shopify.engineering/changing-polymorphic-type-rails which is related

5

phew. I just thought I had been dreaming for a year thinking that we were safe from this problem. Glad to know we are :slightly_smiling_face: At gusto are indeed working on the namespace protection you two discuss here. Hope to be able to share more soon!

6

haha sorry for the alarm :sweat_smile: I just added a note to the message.

Thinking on this problem, I guess packwerk must encourage its usage on the CI, otherwise the class name collision could happen. It is checking more than just that packwerk is correctly used, but it’s also checking app correctness. For example here https://github.com/Shopify/packwerk/blob/main/UPGRADING.md#gem-group is mentioned that the gem now is needed only in development, when it’d be better to mention that the gem is needed both in development and test groups I guess…

Looking forward to what Gusto shares on the topic :muscle:

7

Message originally sent by slack user U783MOJYF8Z

:thinking_face: I was convinced we had guidance on how to use packwerk in CI, but I can’t find it right now. For sure validate should be run in CI.

However, I think this problem should be found in most packwerk commands (check, for example), because it is checked during the initialization of constant_resolver, which is used by check , update_deprecations and others.

In any case validate should be run in CI.

8

Message originally sent by slack user U783MOJYF8Z

If you want to contribute a CI specific section to the docs I’ll do my best to review it in a timely fashion. (I don’t have a lot of time for packwerk right now but this sounds like a small thing that is realistic to get through)