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 helper function to facilitate use of library with oneOf types #403

Merged
merged 1 commit into from
Aug 14, 2024

Conversation

uasouz
Copy link
Contributor

@uasouz uasouz commented Aug 13, 2024

AnyMany thanks for submitting your Pull Request ❤️!

What this PR does / why we need it:
I was using the library and it felt a by unconfortable to check every item on an oneOf if it was null or not to define it inner type so i made a minor tweak on generator and deserializers to haver a hint allowing the use of a switch expression to select the type.

Here follows a example of usage:

switch (taskitem.get()) {
            case CallTask callTask -> {
                switch (callTask.get()) {
                    case CallHTTP httpCall -> {
                        yield activities.execute(HTTP_ACTION_METHOD_NAME, TaskResult.class, execution.getContext(), input);
                    }
                    case CallGRPC grpcCall -> {

                        throw new NotImplementedException();
                    }
                    case CallFunction functionCall -> {

                        throw new NotImplementedException();
                    }
                    case CallOpenAPI openApiCall -> {

                        throw new NotImplementedException();
                    }
                    case CallAsyncAPI asyncApiCall -> {

                        throw new NotImplementedException();
                    }
                    default -> throw new IllegalStateException("Unexpected value: " + callTask.get());
                }
            }
            case SwitchTask switchTask -> {
                throw new NotImplementedException();
            }
            case DoTask doTask -> {

                throw new NotImplementedException();
            }
            case ForkTask forkTask -> {

                throw new NotImplementedException();
            }
            case ForTask forTask -> {

                throw new NotImplementedException();
            }
            case RaiseTask raiseTask -> {

                throw new NotImplementedException();
            }
            case SetTask setTask -> {

                throw new NotImplementedException();
            }
            case TryTask tryTask -> {

                throw new NotImplementedException();
            }
            case WaitTask waitTask -> {

                throw new NotImplementedException();
            }
            case ListenTask listenTask -> {

                throw new NotImplementedException();
            }
            case EmitTask emitTask -> {

                throw new NotImplementedException();
            }
            default -> {
                yield null;
            }
 } 

Special notes for reviewers:

I'm open to comments and reviews and still can contribute on this PR

Additional information (if needed):

@ricardozanini
Copy link
Member

@uasouz thank you! Can u please sign your commit?

@fjtirado
Copy link
Collaborator

@uasouz
I have to admit I wasnt aware of the latest changes on Switch Case statement for Java 17.
Im not sure the code proposed is better than


if (task.getCallTask() != null)  {
   if (task.getCallTask().getHttpCall() != null) {
      ....
  }
}
throw new NotImplementedExceptoin() 

I would say it depends on tastes.
Anyway. I agree we should support the new switch syntax.

I would change the name from getDefinition() to unwrap() or get() or even case(). And probably make the generated class implements an interface with that method name.

And regarding implementation, rather that using instrospection on DeserializateHelper to change the value of the intenal field, I would generate code that assigns the internal field in every constructor.

Copy link
Collaborator

@fjtirado fjtirado left a comment

Choose a reason for hiding this comment

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

As mentioned previously, I would change method name and the deserialization approach.

@uasouz uasouz force-pushed the main branch 3 times, most recently from 56e0074 to 04c3354 Compare August 13, 2024 14:48
@uasouz
Copy link
Contributor Author

uasouz commented Aug 13, 2024

@fjtirado Thanks for the feedback, I will adjust the PR accordingly and send new commits soon.

@ricardozanini Sorry for many force commits, had small issue with bad config not signing with mixed git configs.

@ricardozanini
Copy link
Member

@uasouz please try squashing your commits and sign it.

@uasouz
Copy link
Contributor Author

uasouz commented Aug 13, 2024

The PR is updated.

I squashed the commits and looks like they are signed and verified. But it still fails the check. I have to figure out why. But if there is anything else I can change on the PR, just comment.

@uasouz uasouz requested a review from fjtirado August 13, 2024 19:25
…selecting current type of item instead of checking all of them

Add Interface and Implement it

Add assignment on constructor

Signed-off-by: Vinícius Moraes Lopes <vlopes45@gmail.com>
@ricardozanini
Copy link
Member

@uasouz since this is your first contribution, the checks only happen when I approve the run. It's a security measurement from GH.

@ricardozanini ricardozanini merged commit eb4f2af into serverlessworkflow:main Aug 14, 2024
2 checks passed
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.

3 participants