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

Serializing Descrimination Union Not Selecting Correct Member #101

Open
justin0mcateer opened this issue Nov 4, 2024 · 9 comments
Open
Assignees
Labels
bug Something isn't working v9 Features planned for the v9 version

Comments

@justin0mcateer
Copy link

I have a schema which defines a number of different messages to be used within a protocol. There are individual schema for each message type and a union used by the protocol which includes all message types in the protocol. All of the message schema have a discriminator field (in this case 'type') which indicates the type of message, and by extension the correct/matching schema.

When parsing, this works fine. However, when serializing the wrong schema seems to be selected for the serialization and whether the correct or incorrect schema is selected seems to depend on the ordering in the union.

image

In the Rescript API, there appears to be a special 'tag' construct that allows specifying the discriminator. Other libraries also allow specifying the discriminator for these types of unions. It seems like it would be both more reliable and more performant to leverage an explicit discriminator when available.

@justin0mcateer
Copy link
Author

This is the generated code on that union schema, for whatever reason, it is checking the 'collectionName' field, which is a common field, but not constant and not the discriminator.

image

@DZakh
Copy link
Owner

DZakh commented Nov 5, 2024

Thanks for the report. Could you share your version and the SyncResponseSchema with SyncRequestSchema code.

In the Rescript API, there appears to be a special 'tag' construct that allows specifying the discriminator.

This is the same as a literal shorthand in TS API. This needs to be a function because the ReScript lang type system doesn't allow overloads.

Other libraries also allow specifying the discriminator for these types of unions. It seems like it would be both more reliable and more performant to leverage an explicit discriminator when available.

ReScript Schema does it automatically 🙂 It Looks like, for some reason, the type field is not a literal.

@DZakh DZakh self-assigned this Nov 5, 2024
@justin0mcateer
Copy link
Author

Maybe I am holding it wrong, but I have tried everything I can think of to get this to work. I've tried S.literal, 'as const', I've tried simplifying the schema, changing the name of the discriminator field and so on. The result is always exactly the same.

Here is another interesting test, after some changes to simplify things further, serializing with the Response schema works, but serializing with the Union fails. Again, this only happens if the inputs to Union are in a particular order.

image

Here are the simplified schema:
image

As as short term work around, I am switching on the 'type' field and serializing with the schema for the specific message and that is working fine. Just creates extra code that is harder to maintain.

Also, I notice 'serialize' is marked as deprecated, but I don't see S.serializeWith exposed in the TS API.

@justin0mcateer
Copy link
Author

As for the version, I was on 8.3.0, but I just upgraded to 8.4.0 (latest) to see if that would resolve the issue. Setting aside the API differences, the behavior in this case appears to be exactly the same between 8.3 and 8.4.

@justin0mcateer
Copy link
Author

OK. New information. If I change the 'response' field from a Union to S.string, all the cases work correctly. So, it appears that having a discriminated Union with a field which is also a Union seems to be causing some issue. Notably, it is this field that is being left out when the object is serialized.

I checked with other non-primitive schema properties and they don't seem to trigger the problem.

@DZakh
Copy link
Owner

DZakh commented Nov 5, 2024

Super weird. I'm currently refactoring this code for V9. It's going to be more robust and performant. It's not ready yet, but I can probably cherry-pick some changes and release 8.4.1. I think it might solve the issue.

I'll keep you updated.

@DZakh
Copy link
Owner

DZakh commented Nov 5, 2024

Also, there's a chance that 8.1.0 don't have the bug, but I see that you use some later feature, so it might be difficult for you to downgrade.

@DZakh DZakh added bug Something isn't working v9 Features planned for the v9 version labels Nov 5, 2024
@DZakh
Copy link
Owner

DZakh commented Nov 5, 2024

Yeah, I reproduced the issue. Unfortunately, the cherry-picking is not possible as I initially thought.

For now, I set 8.1.0 as the latest version on npm and have notified people on Twitter to use it until I release v9 with a fix.

https://x.com/dzakh_dev/status/1853855075061367150

DZakh added a commit that referenced this issue Nov 5, 2024
DZakh added a commit that referenced this issue Nov 24, 2024
@DZakh
Copy link
Owner

DZakh commented Dec 6, 2024

Published https://github.com/DZakh/rescript-schema/releases/tag/v9.0.0-rc.1 with a fix.

It still requires to write a change log and update documentation, but the code isn't going to be changed until the official v9 release.

DZakh added a commit that referenced this issue Dec 15, 2024
DZakh added a commit that referenced this issue Dec 15, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working v9 Features planned for the v9 version
Projects
None yet
Development

No branches or pull requests

2 participants