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

Make classes serializable #283

Merged
merged 1 commit into from
May 27, 2024

Conversation

visheshruparelia
Copy link
Contributor

What this PR does / why we need it:
There were some manually created POJOs which did not implement serializable interface. Making those implement Serializable interface here. This will enable Workflow objects transferrable over wire.

Signed-off-by: Vishesh Ruparelia <[email protected]>
@visheshruparelia
Copy link
Contributor Author

@ricardozanini just curious: I saw most of the classes are generated automatically from the JsonSchema but some of them are manually created. These manually created ones did not implement Serializable interface. I wonder why did we create these classes manually?

@visheshruparelia
Copy link
Contributor Author

I have tested this branch (synced with latest main branch) on my local there is no regression and also my usecase is now working after this.
Quick question: Are there any other classes which were manually written apart from the ones present in this package? In that case we will have to make those classes implement serializable too.
NOTE: I am only worried about the classes being used inside the main Workflow class other Util classes are fine.

@ricardozanini
Copy link
Member

Quick question: Are there any other classes which were manually written apart from the ones present in this package? In that case we will have to make those classes implement serializable too.

The manually created classes are needed in order to implement a custom behavior that the generated ones cannot.

I'd recommend reviewing the whole project and writing more throughout tests using coverage to make sure everything is set before we merge this one. Can you do it?

@visheshruparelia
Copy link
Contributor Author

Sure. As of now I am using a workaround to solve my usecase so not a blocker for me.
It will take some time for me though.

Just a thought: How do you propose testing this in UTs?

@visheshruparelia
Copy link
Contributor Author

Another option is we merge this and I work on a follow up PR which does a comprehensive check as this PR alone seems harmless to me does not cause any regression.
Upto you...

@ricardozanini
Copy link
Member

I'd rather everything in one PR if you don't mind since I have to cherry pick to another branch to do a fup release.

@ricardozanini
Copy link
Member

@visheshruparelia I'm merging this one to help with your use case, sorry for the late merging. We will release a new major version targeting Java 11, so you can go from there.

@ricardozanini ricardozanini merged commit 67a6786 into serverlessworkflow:main May 27, 2024
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants