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

Add serde interfaces #5893

Merged
merged 6 commits into from
Mar 24, 2025
Merged

Add serde interfaces #5893

merged 6 commits into from
Mar 24, 2025

Conversation

pditommaso
Copy link
Member

@pditommaso pditommaso requested a review from jorgee March 18, 2025 11:28
@pditommaso pditommaso marked this pull request as draft March 18, 2025 11:28
@jorgee
Copy link
Contributor

jorgee commented Mar 21, 2025

Update:

  • Changes in the CidObserver, CidOperationImpl, and CidPath to support the new interface.
  • Restrict the encoder interface to encode/decode CidSerializable objects.
  • Fix tests

Note: I have restricted the encoder interface because for CIDs we do not need a generic input type. Also when loading the object from the CidStore in CidPath or the CidOperationImpl classes, we don't know the exact model class that we expected, it is only known data is retrieved and the type field is inspected, we only know they are CidSerializable and the Groovy's JsonSlurper was not able to cast to an interface. So, I needed to add something to manage the polymorphism. Currently, I have implemented a simple mechanism in the decode method including in the DataType enum the class to cast the retrieved object. We could manage more generically with Moshi Adapters as done in Wave but we must add a new dependency and it is not required for the Cid case. I can do it later if we want to create a generic encoder to be used in other part in Nextflow.

pditommaso and others added 2 commits March 21, 2025 10:47
Signed-off-by: Paolo Di Tommaso <paolo.ditommaso@gmail.com>
Signed-off-by: jorgee <jorge.ejarque@seqera.io>
@pditommaso
Copy link
Member Author

Just update the cid-store branch, let's solve the conflicts

Signed-off-by: jorgee <jorge.ejarque@seqera.io>
Signed-off-by: Paolo Di Tommaso <paolo.ditommaso@gmail.com>
@pditommaso pditommaso marked this pull request as ready for review March 22, 2025 10:00
@pditommaso
Copy link
Member Author

Went ahead with the suggestion for a generic serde. Main changes:

  • Move the base encoder interface and logic in nf-commons
  • Implement a json encoder based on Gson that's already included in the nextflow base classpath
  • Refactor CidEncoder based on GsonEncoder
  • Remove the need for an *explicit* type field CID model objects.
  • Use consistently CidEncoder in place of JsonOutput.

Signed-off-by: Paolo Di Tommaso <paolo.ditommaso@gmail.com>
Signed-off-by: jorgee <jorge.ejarque@seqera.io>
Copy link
Contributor

@jorgee jorgee left a comment

Choose a reason for hiding this comment

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

Everything works as expected. I have just renamed the test class JsonEncoderTest to CidEncoderTest to match with the refactor. Ready to merge to cid-store from my side. Once merged, I will update the annotations PR

@pditommaso pditommaso merged commit db79c43 into cid-store Mar 24, 2025
5 of 9 checks passed
@pditommaso pditommaso deleted the cid-serde branch March 24, 2025 10:46
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.

2 participants