[WIP] Ensure references_many no nil value

[jkeuleya]

Issue:

When we have a deserializer with a references_many inside and that we don't receive this value in the response of the API call, it crashes.

What we want:

When we have a deserializer with a references_many inside and that we don't receive this value in the response of the API call, it should just ignore it and continue reading next attributes as it works for references_one and attribute.

What's happening concretely in the code:

While deserializing, while arriving on a non existing reference_many in the response, we .assign_attribute the reference_many with a nil value. This causes the crash (lib/ledger_sync/deserializer.rb:44).

What's the plan:

Maybe try to set the value to [] (empty array) when the reference is not present in the response.

Example:

When I receive the answer from my API call on AmazonBusinessAPI, sometimes, it doesn't have the "packages" key with an array as value, like that:

# Response from API with `packages`
deserializer = AmazonBusinessApi::AcceptanceArtifact::Deserializer.new
response = { "identifier" => "42", "packages" => [] }
accept_art = deserializer.deserialize(hash: response, resource: AmazonBusinessApi::AcceptanceArtifact.new)
=> Success

# Response from API call without `packages`
deserializer = AmazonBusinessApi::AcceptanceArtifact::Deserializer.new
response = { "identifier" => "42" }
accept_art = deserializer.deserialize(hash: response, resource: AmazonBusinessApi::AcceptanceArtifact.new)
=> .../ledger_sync/lib/ledger_sync/error/resource_errors.rb:23:in `generate_message': undefined method `reject' for nil:NilClass (NoMethodError)
=> invalid_classes = value.reject { |e| e.is_a?(type_resource_class) }.map(&:class)

In the first case, everything works fine. In the second case, the gem searches for packages, doesn't find it and crashes.

[jkeuleya]

I took 2 hours to check files and write the fix. I didn't check the whole code base. It's possible that I missed some things here. I didn't want to take too much time on this part since I have other tasks to do in this sprint.

Waiting for your review 😄

[jkeuleya]

Basically, when an array is not received in the answer (in my case it's amazon business api), references_many is nil. Its value is nil too. I ensure here that if we meet this specific case, we set an empty array instead [].

Why here? Because after, comes the .set_attribute(key, value) and it can't be nil.

I checked files and methods before arriving here. I followed manually the path taken by the code execution. I think that this file is the best place to check value's presence. Other files are even more generic and not appropriate imo.

[jozefvaclavik]

Hey @jkeuleya. Thx for working on this. I'm afraid that this PR is trying to fix it on a wrong layer. Lets try something else here. From what I tracked down is that during deserialization we get the value from hash and then we cast it on type in:

ledger_sync/lib/ledger_sync/serialization/deserializer_attribute.rb

Line 36 in bc868e9

value = type.cast(value: value)

from there we exit early in cast method because value is nil in:

ledger_sync/lib/ledger_sync/type/value_mixin.rb

Line 25 in bc868e9

return nil if args.fetch(:value).nil?

.

So the DeserializerReferencesManyType and its cast_value method are never called:

ledger_sync/lib/ledger_sync/serialization/type/deserializer_references_many_type.rb

Line 14 in bc868e9

return if value.nil?

As this is specific just for this type, that feels like its the right place to handle it.

Lets remove the exit for nil value in cast method as I believe the type is responsible how the value (even nil) needs to be casted. And then in DeserializerReferencesManyType instead of returning nil if value is nil, lets return empty array [] instead.

Let me know if this approach can work and resolves the issue.

[ryanwjackson]

@jkeuleya can you also write a test for this so we ensure we are capturing the expected behavior? That would help me know what you're ultimately trying to achieve.

If I am understanding correctly, I'm concerned that this change would be converting nil to [] which I don't think is the correct behavior.

It may be easiest to just have a mock of the Amazon Business API response that is failing to create a feature test.

[jkeuleya]

Thank you everyone for your time. I updated the description with an example.
I'll provide you today or Monday a spec if possible and go through @jozefvaclavik 's proposition !

EDIT:

Classes + spec ready to review.
Will push a fix ASAP.

The current PR should help to locate and understand the issue

[ryanwjackson]

What if we add a param called default to references_many so that you could do default: []? I worry that your implementation here is going to be a breaking change. Unless I am missing something, I believe one could argue either way that an API not returning a parameter should result in an empty list or nil. So I think we should make it configurable to make this change backwards compatible.

@jozefvaclavik do you agree?

[jkeuleya]

It's exactly what I searched at the beginning, a default. It was the easy solution but it doesn't exist yet. So yes, it could've been a solution.

For my first implementation, yes, it was a naive approach, I didn't take time to check the whole code base. My goal was to even avoid default param and just do a presence check.

But still, need to think about the implementation of a default while not creating a breaking change.. 🤔

[jkeuleya]

In this proposition (thanks @jozefvaclavik for your time and your research), we're being more precise/meticulous.

We're working directly on the references_many file.

Nonetheless, we also touched the value_mixin file (see below) which could lead to edge cases since it's a transversal file. I cannot guarantee here. I did ofc several tests with several values and types. I put some breaking points in the code execution trying to understand and check. But still.

[jkeuleya]

We remove here this responsibility to cast method.