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

Fix for #376 #377

Merged
merged 2 commits into from
Jun 28, 2020
Merged

Fix for #376 #377

merged 2 commits into from
Jun 28, 2020

Conversation

tkalmar
Copy link

@tkalmar tkalmar commented Jun 23, 2020

this is a first try at fixing #376

@phillip-kruger
Copy link
Member

@tkalmar Thanks for this. I think something went wrong with you PR, it shows 300+ file changes ?

@tkalmar tkalmar force-pushed the master branch 2 times, most recently from 6980c39 to e8c3f7f Compare June 23, 2020 10:41
@tkalmar
Copy link
Author

tkalmar commented Jun 23, 2020

@phillip-kruger reformat the PR so only the new files are actualy new ... now it even builds

@phillip-kruger phillip-kruger added this to the 2.0.3 milestone Jun 23, 2020
MikeEdgar
MikeEdgar previously approved these changes Jun 24, 2020
Copy link
Member

@MikeEdgar MikeEdgar left a comment

Choose a reason for hiding this comment

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

I think this code should ultimately be in the ParameterProcessor where more context is known about the parameter. I noted that this approach may not work well outside of the basic situation where the only arguments to the resource method are non-form @*Param types.

We already have #267 dealing with @Extension, so perhaps this could be updated as well when that issue is fixed.

Comment on lines 636 to 638
final MethodParameterInfo methodParameterInfo = annotation.target().asMethodParameter();
final short paramPosition = methodParameterInfo.position();
operation.getParameters().get(paramPosition).addExtension(name, value);
Copy link
Member

Choose a reason for hiding this comment

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

I don't think this approach is going to work outside of the basic scenario. I.e., the position in the operation's parameters may not always be the same as the position in the method parameters. Think of the case where @BeanParam is used or where parameters are injected into the resource class itself rather than as method arguments.

Copy link
Member

Choose a reason for hiding this comment

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

@MikeEdgar - Any suggestion on how @tkalmar can change the PR so that this will work better ?

@MikeEdgar MikeEdgar linked an issue Jun 24, 2020 that may be closed by this pull request
@MikeEdgar
Copy link
Member

@tkalmar , @phillip-kruger - I think a good approach would look something like this. I haven't tested this out :-)

  1. io.smallrye.openapi.runtime.io.extension.ExtensionReader: overload readExtensions method, pass AnnotationScannerContext and List<AnnotationInstance>. Returns a Map<String, Object>, same as existing readExtensions by iterating over the List and creating a map entry for each list entry.
  2. Pass AnnotationScannerContext from JaxRsAnnotationScanner to ParameterProcessor and set as an instance variable (the Index parameter can be removed and set the instance variable from the AnnotationScannerContext in the ParameterProcessor constructor. The calls from JaxRsAnnotationScanner can simply pass the context instead of context.getIndex().
  3. Read the extensions in Parameter processor after the code for setDeprecated. Something like below. In this example, this.scannerContext is the new instance field from change (2) above.
List<AnnotationInstance> extensionAnnotations = ExtensionReader.getExtensionsAnnotations(context.target);

if (param.getExtensions() == null && !extensionAnnotations.isEmpty()) {
    param.setExtensions(ExtensionReader.readExtensions(this.scannerContext, extensionAnnotations));
}

@MikeEdgar MikeEdgar dismissed their stale review June 27, 2020 15:28

Dismissing earlier approval in favor of the changes detailed above.

@MikeEdgar MikeEdgar self-requested a review June 27, 2020 15:28
Copy link
Member

@MikeEdgar MikeEdgar left a comment

Choose a reason for hiding this comment

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

See earlier comments regarding changes in the ParameterProcessor

@tkalmar
Copy link
Author

tkalmar commented Jun 27, 2020

@phillip-kruger @MikeEdgar Thanks for the help, i extended the test to include one @BeanParam and it broke as expected. With the suggested changes it was a piece of cake to do the required changes. Test works now and is better than my first draft. Thanks a lot.

@tkalmar tkalmar force-pushed the master branch 6 times, most recently from b0fac5c to d656d17 Compare June 27, 2020 18:35
Copy link
Member

@MikeEdgar MikeEdgar left a comment

Choose a reason for hiding this comment

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

Looks good, thanks @tkalmar. Any concerns @phillip-kruger ?

@tkalmar
Copy link
Author

tkalmar commented Jun 28, 2020

Should this be backported at the 1.x branch to get it faster into quarkus or are there plans to switch quarkus to the 2.x branch in the near future?

@phillip-kruger
Copy link
Member

Quarkus is on 2.x starting 1.6. we can try and get it in a.s.a.p. no need to backport.

@phillip-kruger phillip-kruger merged commit 8db0fdc into smallrye:master Jun 28, 2020
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.

@Extension on pathParam is added to operation
3 participants