graphql-platform icon indicating copy to clipboard operation
graphql-platform copied to clipboard

Support ID list deserialization with custom Ids

Open faddiv opened this issue 2 years ago • 3 comments

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

faddiv avatar Feb 18 '24 18:02 faddiv

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

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%) :arrow_up:

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

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

codecov[bot] avatar Feb 18 '24 19:02 codecov[bot]

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 avatar Feb 19 '24 14:02 faddiv

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 avatar Feb 24 '24 19:02 faddiv

Hi Michael, As I see this is solved in this PR: https://github.com/ChilliCream/graphql-platform/pull/7026 so I closed.

faddiv avatar Apr 06 '24 20:04 faddiv