[GitHub] [geronimo-openapi] jgallimore opened a new pull request #20: Skip @RegisterRestClient

classic Classic list List threaded Threaded
5 messages Options
Reply | Threaded
Open this post in threaded view
|

[GitHub] [geronimo-openapi] jgallimore opened a new pull request #20: Skip @RegisterRestClient

GitBox

jgallimore opened a new pull request #20:
URL: https://github.com/apache/geronimo-openapi/pull/20


   When an implementation of MicroProfile REST client is available on the classpath, the AirlinesAppTest TCK test fails, as the `org.eclipse.microprofile.openapi.apps.airlines.resources.PlayerService` bean is incorrectly included in the set of endpoints.
   
   Test failure details:
   
   ```
   [ERROR] Failures:
   [ERROR] org.eclipse.microprofile.openapi.tck.AirlinesAppTest.testRestClientNotPickedUp(org.eclipse.microprofile.openapi.tck.AirlinesAppTest)
   [ERROR]   Run 1: AirlinesAppTest>Arquillian.run:138->testRestClientNotPickedUp:191 1 expectation failed.
   JSON path paths.'/player/{playerId}' doesn't match.
   Expected: null
     Actual: {get={operationId=getPlayerById, responses={200={description=default response, content={application/json={schema={type=string}}}}, default={description=default response, content={application/json={schema={type=string}}}}}, parameters=[{schema={type=string}, in=path, name=playerId, style=simple, required=true}]}}
   
   [ERROR]   Run 2: AirlinesAppTest>Arquillian.run:138->testRestClientNotPickedUp:191 1 expectation failed.
   JSON path paths.'/player/{playerId}' doesn't match.
   Expected: null
     Actual: {get={operationId=getPlayerById, responses={200={description=default response, content={application/json={schema={type=string}}}}, default={description=default response, content={application/json={schema={type=string}}}}}, parameters=[{schema={type=string}, in=path, name=playerId, style=simple, required=true}]}}
   
   ```


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
[hidden email]


Reply | Threaded
Open this post in threaded view
|

[GitHub] [geronimo-openapi] rmannibucau commented on pull request #20: Skip @RegisterRestClient

GitBox

rmannibucau commented on pull request #20:
URL: https://github.com/apache/geronimo-openapi/pull/20#issuecomment-623553039


   Hi @jgallimore ,
   
   most of our users don't use mp-rest-client (plain JAXRS 2 client AFAIK) so I think we shouldnt import it and break if not in the classpath (extension code).
   
   There is also a case MP forget there: you create a contract (as an interface), create the dynamic client (@RegisterRestClient) from it AND implement it (server so it must end up in openapi) - this is a not that rare pattern in uservice proxies these days. This change breaks this use case I think.
   
   
   I see multiple options there (none perfect):
   
   1. configure the tck through standard MP openapi excludes config entry (think it is not insane by itself to ask the user to exclude a bean he does not want in the openapi.json)
   2. (likely very tomee oriented) make findEndpointsAndApplication (or new method) protected and in tomee you skip default impl in favor of a subclass
   3. add config to enrich the ignore logic
   4. add a SPI to ignore bean, it can take the form of a fireEvent(new FindFilter()) in BeforeBeanDiscovery of the extension, another extension will observe it do an event.registerFilter(Predicate<AnnotatedType<?>>) and use that in findEndpointsAndApplication
   
   side note: not very important and unrelated to this particular pr but think we should move the typename check before the annotation check no?
   
   personally I think 1 is sane but I can understand 4 makes sense too. I'm not a fan at all of 3 since it will make it more complex for a poor gain IMHO.
   
   Hope it makes sense.
   


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
[hidden email]


Reply | Threaded
Open this post in threaded view
|

[GitHub] [geronimo-openapi] jgallimore commented on pull request #20: Skip @RegisterRestClient

GitBox
In reply to this post by GitBox

jgallimore commented on pull request #20:
URL: https://github.com/apache/geronimo-openapi/pull/20#issuecomment-623555356


   Hey @rmannibucau , thanks for the review!
   
   Could I do option (2) and send a PR for that? Making that method protected would likely be perfect for what I need to do, is likely the simplest change.


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
[hidden email]


Reply | Threaded
Open this post in threaded view
|

[GitHub] [geronimo-openapi] jgallimore commented on pull request #20: Skip @RegisterRestClient

GitBox
In reply to this post by GitBox

jgallimore commented on pull request #20:
URL: https://github.com/apache/geronimo-openapi/pull/20#issuecomment-623558249


   PR for option (2) here: https://github.com/apache/geronimo-openapi/pull/21


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
[hidden email]


Reply | Threaded
Open this post in threaded view
|

[GitHub] [geronimo-openapi] rmannibucau commented on pull request #20: Skip @RegisterRestClient

GitBox
In reply to this post by GitBox

rmannibucau commented on pull request #20:
URL: https://github.com/apache/geronimo-openapi/pull/20#issuecomment-623559846


   Hehe, indeed, was the option "up to you" ;)


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
[hidden email]