[Fix] Conflict when root and object property share the same name #99
Conversation
| carrier.camelize ? Surrealist::HashUtils.camelize_hash(hash) : hash | ||
| copied_schema = Surrealist::Copier.deep_copy(schema) | ||
| built_schema = Builder.new(carrier, copied_schema, instance).call | ||
| wrapped_schema = Surrealist::Wrapper.wrap(built_schema, carrier, instance.class.name) |
There was a problem hiding this comment.
This code makes me feel a bit anxious, maybe we should encapsulate this via something like a command pattern here?
There was a problem hiding this comment.
Yes, you are right, and it would be very nice of you to refactor this part a bit.
| @@ -3,6 +3,8 @@ | |||
| module Surrealist | |||
| # A helper class for hashes transformations. | |||
| module HashUtils | |||
| EMPTY_HASH = {}.freeze | |||
There was a problem hiding this comment.
HashUtils seems like a more obvious place to store this.
|
@akxcv thank you for the PR and such a detailed explanation! I am vacationing right now, so I'll review this next week |
| carrier.camelize ? Surrealist::HashUtils.camelize_hash(hash) : hash | ||
| copied_schema = Surrealist::Copier.deep_copy(schema) | ||
| built_schema = Builder.new(carrier, copied_schema, instance).call | ||
| wrapped_schema = Surrealist::Wrapper.wrap(built_schema, carrier, instance.class.name) |
There was a problem hiding this comment.
Yes, you are right, and it would be very nice of you to refactor this part a bit.
| @@ -3,6 +3,8 @@ | |||
| module Surrealist | |||
| # A helper class for hashes transformations. | |||
| module HashUtils | |||
| EMPTY_HASH = {}.freeze | |||
| # @param [Bool] namespaces_condition whether to wrap into namespace. | ||
| # | ||
| # @return [Hash] deeply copied hash, possibly wrapped. | ||
| def possibly_wrapped_hash(hash, klass, carrier, namespaces_condition) |
There was a problem hiding this comment.
Well, this also doesn't look good in fact. Could you please leave a TODO note to refactor this later?
| }, | ||
| }, | ||
| } | ||
| end |
There was a problem hiding this comment.
Could you please add spaces between lets?
|
hey @akxcv, any updates on this? |
|
@nesaulov I've fixed everything but the first of your comments on my machine. Thinking about how to refactor this part better. |
|
Added a TODO comment. Let's release the fix already :) |
Fixes #95.
Why does it fail?
First, we normalize the schema, which includes wrapping it in a root, if given. (This also happens with namespaces.)
Then, the normalized schema is passed to the Builder along with the instance (from the example given in #95, we get something like
instance: #<Foo:0x00007fb2651a1db8>, schema: { foo: { foo: String, bar: String } }Afterward, the Builder iterates through the schema's key-value pairs. When
instance.respond_to?(:foo), the Builder calls itself again, this time withinstance.fooas the instance and{ foo: String, bar: String }as the schema. You should see the problem now.My proposed fix
I found that the builder only needs to know the actual schema, without any roots or namespaces. So, the current algorithm of serialization is
copy -> build -> wrapinstead ofcopy&wrap -> build.I've split
CopierintoCopierandWrapper, which should make their respective responsibilities clearer.P.S I couldn't come up with any good real-world examples of this use-case for the specs 😞