Closed swilgosz closed 1 year ago
I've investigated the topic a little bit more.
https://github.com/rom-rb/rom-factory/blob/main/lib/rom/factory/factories.rb#L135
In case, the name is a hash (parent is defined), all options are ignored. I guess it's not a big issue, name is a hash only if the parent is defined, and then the rest of options just inherit from parent.
However, this is inconsistent behaviour and I guess it was not intended, but rather overlooked.
I've fixed this: https://github.com/rom-rb/rom-factory/compare/main...swilgosz:rom-factory:fix/top-level-custom-struct-recognition?expand=1
When we call the builder with struct_namespace: nil
or struct_namespace: false
, the default struct namespace is pulled from Factories
config: https://github.com/rom-rb/rom-factory/blob/main/lib/rom/factory/factories.rb#L209
This makes it impossible to override the struct_namespace with nil
, because it'll always fall-back into ROM::Struct
Also fixed in: https://github.com/rom-rb/rom-factory/compare/main...swilgosz:rom-factory:fix/top-level-custom-struct-recognition?expand=1
The above did not fix the test I wrote, because calling relation.struct_namespace(nil)
, will still set the model on the mapper into ROM::Struct
This, however, is not a behavior of ROM-Factory, but rather problem with relations auto-struct. I got lost when I started to dig into ROM::Relations, so I'd need some help to point me into the right place. cc: @flash-gordon
This is just not supported. struct_namespace is always meant to be set to something. We can tweak it to raise an error if nil is passed.
Hm... That's sad. I raised it, becasue hanami-model requires Top-level constants to be entities, and it makes it harder to upgrade big Hanami 1 applications.
Thanks for this, I'll try another approach, tweaking hanami-model instead.
BTW: @solnic I guess the easiest would be to patch the documentation instead :).
@swilgosz hmm, what if you set struct_namespace(Object)
?
I tried that first, there was no effect, but you are the second person suggesting that. I will briefly try again woth some more detailed look tomorrow and will get back to you, thanks!
@solnic It worked! It required from me to also add struct_namespace: Object
in the repositories, but it actually worked. Thanks!
@swilgosz ah yeah, repositories configure their relations to use a namespace so this makes sense. I'm glad that it worked 🙂
Describe the bug
We can define custom structs nested in a
module
, and then pointing to it viastructs_namespace
option. It works fine, but it does not work with top-level custom struct classes.Reasoning: It makes it problematic to update Hanami 1 applications to Hanami 2, because Hanami-Model (Hanami 1) assumed top-level entities to be defined.
Other than that, this behaviour is inconsistent.
To Reproduce
I've written rspec examples for this usecase: https://github.com/swilgosz/rom-factory/commit/b0afeb720110a28104ebc573a935311f4892f942
Expected behavior
I expect that either, by passing
struct_namespace: nil
or by default, ROM-Factory will try to call the top-level structs if they are defined. It should fallback toROM::Struct::[[FactoryClass]]
in the constant is not found, or is not a kind ofROM::Struct
My environment