Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support ID list deserialization with custom Ids #6918

Closed
wants to merge 0 commits into from

Conversation

faddiv
Copy link

@faddiv faddiv commented Feb 18, 2024

Hi,
We ran into this problem when we tried to use plural custom id-s.

  • CreateListFactory for custom types creates a string list instead of a custom type list. From this, the type converter can pick up it and create the correct type.

I used one of the recommendations from this PR to solve the issue.

Closes #5114

Copy link

codecov bot commented Feb 18, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 73.14%. Comparing base (1865894) to head (08ef4b2).

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #6918      +/-   ##
==========================================
+ Coverage   70.56%   73.14%   +2.58%     
==========================================
  Files        2597     2583      -14     
  Lines      129007   128371     -636     
==========================================
+ Hits        91028    93893    +2865     
+ Misses      37979    34478    -3501     
Flag Coverage Δ
unittests 73.14% <100.00%> (+2.58%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Member

@michaelstaib michaelstaib left a comment

Choose a reason for hiding this comment

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

Let me check why the type conversion does not work with typed lists ... I think the change should not be needed.

@faddiv
Copy link
Author

faddiv commented Feb 19, 2024

Let me check why the type conversion does not work with typed lists ... I think the change should not be needed.

It doesn't work, because the createList creates a list with concrete type. In this case: new List<CustomId>()
but the id serializer serializes the CustomId with (d)efault type; when it deserializes it, string type is used. This results in error.
The type conversion happens after the ID deserialization, so that can't help. The old PR tried to bring in the conversion, but that would change the public API.

@faddiv
Copy link
Author

faddiv commented Feb 24, 2024

Hi Michael,
If you have time, please consider whether my implementation helps improve HotChocolate.
As far as I can tell, CustomId parsing in List<> is only possible this way since the conversion happens later.
I improved my solution. Now it works even if the serialized ID is not the default type.
I investigated the serializing side also, and there the non-default serialization only happens if a custom resolver is created, which transforms the value to one of the basic types.

@faddiv
Copy link
Author

faddiv commented Apr 6, 2024

Hi Michael,
As I see this is solved in this PR: #7026 so I closed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support Relay ID deserialization with custom Ids
2 participants