Skip to content
This repository was archived by the owner on Oct 27, 2025. It is now read-only.
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ def cast_value(args = {})
value = args.fetch(:value)
resource = args.fetch(:resource)

return if value.nil?
return [] if value.nil?
Copy link
Author

@jkeuleya jkeuleya Sep 2, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


first_dot = deserializer_attribute.resource_attribute_dot_parts.first.to_sym
nested_resource = resource.class.resource_attributes[first_dot].type.resource_class.new
Expand Down
1 change: 0 additions & 1 deletion lib/ledger_sync/type/value_mixin.rb
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,6 @@ def assert_valid(args = {})
# Do not override this method. Override private method cast_value
def cast(args = {})
assert_valid(args)
return nil if args.fetch(:value).nil?
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We remove here this responsibility to cast method.


cast_value(args)
end
Expand Down
65 changes: 65 additions & 0 deletions spec/deserializer/acceptance_artifact_spec.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,65 @@
# Usage: bundle exec rspec spec/deserializer/acceptance_artifact_spec.rb

require 'ledger_sync'
require 'rspec'

class Package < LedgerSync::Resource
attribute :blabla, type: LedgerSync::Type::String
end

class Package
class Deserializer < LedgerSync::Deserializer
attribute :blabla, hash_attribute: :blabla
end
end

class AcceptanceArtifact < LedgerSync::Resource
attribute :identifier, type: LedgerSync::Type::String
references_many :packages, to: Package
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.. 🤔

end

class AcceptanceArtifact
class Deserializer < LedgerSync::Deserializer
attribute :identifier, hash_attribute: :identifier
references_many :packages, deserializer: Package::Deserializer, hash_attribute: :packages
end
end

RSpec.describe AcceptanceArtifact::Deserializer do
describe '#deserialize' do
let(:deserializer) { described_class.new }
let(:acceptance_artifact) { AcceptanceArtifact.new }

subject(:deserialized_object) { deserializer.deserialize(hash: response, resource: acceptance_artifact) }

context "with empty `packages` in the API's response" do
let(:response) { { "identifier" => "42", "packages" => [] } }

it 'deserializes the identifier correctly' do
expect(deserialized_object.identifier).to eq("42")
end
end

context "with filled `packages` in the API's response" do
let(:response) { { "identifier" => "42", "packages" => [ { "blabla" => "ok" } ] } }

it 'deserializes the identifier correctly' do
expect(deserialized_object.identifier).to eq("42")
end
end

context "without `packages` in the API's response" do
let(:response) { { "identifier" => "42" } }

it 'deserializes the packages correctly' do
expect(deserialized_object.packages).to eq([])
end
end
end
end

# Manual trigger
# deserializer = AcceptanceArtifact::Deserializer.new
# response_1 = { "identifier" => "42", "packages" => [] }
# response_2 = { "identifier" => "42" }
# accept_art = deserializer.deserialize(hash: response_1, resource: AcceptanceArtifact.new)