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.

Sign up for GitHub

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

Jump to bottom

Return deserialize_bytes() in CidDeserializer::deserialize_any #21

Closed
wants to merge 1 commit into from
Closed

Return deserialize_bytes() in CidDeserializer::deserialize_any #21

wants to merge 1 commit into from

Conversation

sugyan
Copy link
Contributor

@sugyan sugyan commented Feb 4, 2024

When deserializing data containing cid as an enum, there was no problem if it was Externally tagged, but an Error was returned when it was Internally tagged or Untagged.
This seems to be because the serde will call deserialize_any in the Internally tagged/Untagged case. That caused CidDeserializer::deserialize_any to return an Error when called.

In reality, however, CidDeserializer is only used from Deserializer::deserialize_cid. I thought that when this CidDeserializer is used, the input that should be processed is the bytes of the cid, and there should be no problem processing it with deserialize_bytes. Returning self.deserialize_bytes(visitor) does not affect the results of the existing tests.

It is possible that I am not understanding this well enough, but is there any reason why I should return an Error here?

@vmx
Copy link
Member

vmx commented Feb 5, 2024

Thanks for the PR and the additional tests. I currently work on an serde_ipld_dagjson which might result in some changes, also how CIDs are treated in Serde. Hence I won't review this PR now, but I'll make sure that your tests will also pass in case there is a new version (or in case there won't be a new version, then I'll review this PR properly).

vmx added a commit to ipld/serde_ipld_dagjson that referenced this pull request Feb 6, 2024
@vmx
broken tests
1dec89a 
I ported tests from ipld/serde_ipld_dagcbor#21 which show
issues with the CID as enum approach.
vmx added a commit to ipld/serde_ipld_dagjson that referenced this pull request Feb 6, 2024
@vmx
broken tests
5c39573 
I ported tests from ipld/serde_ipld_dagcbor#21 which show
issues with the CID as enum approach.

Run:

    cargo test --test de test_cid_in_tagged_enum
@sugyan sugyan mentioned this pull request Feb 8, 2024
Closed
@vmx
Copy link
Member

vmx commented Feb 13, 2024

I've created #22, which also contains an explanation on why we current error on deserialize_any. I hope the examples in that PR help for your use case. In case they don't feel free to post your use case here and I can have a look.

@vmx
Copy link
Member

vmx commented Feb 13, 2024

@sugyan I just saw the referenced PR from your repo. It looks like you need internally tagged and untagged enums a lot. I'll have a look if there might be a way to make this smoother and not having to implement it manually for all of them.

@vmx
Copy link
Member

vmx commented Feb 13, 2024

@sugyan I had a closer look at Atrium. I want to find a solution that works for all use cases. It would be a shame if Atrium couldn't use this library as I really strive for having a common library for DAG-CBOR in Rust. I hope we'll find a solution, I really don't want that you'd need to use something custom or use a fork.

@sugyan
Copy link
Contributor Author

sugyan commented Feb 14, 2024

@vmx Thank you for taking a look at ATrium.
In the ATrium use case, the defined structs are basically handled in JSON format using serde_json, but in some situations, such as in the case of subscription messages, the data is handled in DAG-CBOR format.
Since so many types are defined, it is ideal not to have to implement a custom deserializer for each one. I sent this pull-request because I thought serde_ipld_dagcbor should be used to handle DAG-CBOR data, including CIDs.
Is there any way to achieve this other than forking and using a modified version?

@vmx
Copy link
Member

vmx commented Feb 14, 2024

@sugyan here's another try, solving it with adding a feature flag: #23
I need to check with other users of this library if that's an option.

@sugyan
Copy link
Contributor Author

sugyan commented Feb 15, 2024

@vmx I see, it looks good to be able to switch the behavior with the feature flag like that. For my use case of ATrium, I'll be able to leave it with the default feature and it will work fine.
I would like to switch to using the new version when it is merge and released as a new version. Thank you very much!

@vmx
Copy link
Member

vmx commented Feb 23, 2024

#23 was merged. So you should be able to use it now.

There will be one more change I'd like to make before cutting a new release (as it's a breaking one), but that should hopefully happen soon enough.

@vmx
Copy link
Member

vmx commented Mar 26, 2024

This was fixed with the 0.5.0 release with the introduction of the no-cid-as-bytes feature flag, which is not enabled by default.

@vmx vmx closed this Mar 26, 2024
@sugyan sugyan mentioned this pull request Aug 4, 2024
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@sugyan @vmx