juneau-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From David Goddard <godd...@acm.org>
Subject Re: Swagger in RestMethod (7.2.0 -> 7.2.1 and onwards)
Date Fri, 26 Oct 2018 09:03:38 GMT
On 25/10/2018 22:52, James Bognar wrote:

> I could use some guidance here actually on whether to keep 
> the @Parameter annotation.  The plan was to deprecate it, but I'm 
> willing to keep it in.

I prefer to remove ambiguity so in the long term support removing it.

> The "new" approach is to annotate your parameters on the method 
> arguments like so:
> 
> @RestMethod(name="GET",
>      path="/foo/{somePathParam}/*",
>      properties ={
>          @Property(name=HTMLDOC_header, value="$L{title}")
>      }
> )
> public ServiceResult someMethod(
>          RestRequest request,
>          RestResponse response,
>          @Path(
>              name="somePathParam",
>              description="xxx"
>          )
>          String somePathParam) {
>         ...
>     }
> 

OK, this is good - works for me and is a clean approach, thanks.

[...]
> As far as keeping @Parameter, I could re-add it using a separate 
> "parameters2" attribute like so:
> @RestMethod(name="GET",
>      swagger=@MethodSwagger(
>          parameters2={
>              @Parameter(....)
>          }
>      )
> )
 >
> My concerns with that though are that we're defining two separate ways 
> of defining free-form parameters.  And the existing @Parameter 
> annotation didn't get the Swagger spec exactly correct (it was an 
> amalgam of "body" and the other parameter types which only has some 
> overlap in Swagger).

This would still be a breaking change though?  As it understand it, 
@Parameter is already back in the 7.2.1 code, so as not to break old 
code.  For a future major release, I would support removing it.  I agree 
that we should not have too many ways of defining these params so it 
should go entirely, as long as it's a major release with a release note 
describing an 'old' and 'new' way of doing it, so migration is clear.

> I'm not sure why you're seeing that "@Path used without name or value" 
> error.  I'll see if I can reproduce that.  The workaround is to add the 
> name of your path parameter like so:
> 
> public ServiceResult someMethod(
>      RestRequest request,
>      RestResponse response,
>      @Path("somePathParam") String somePathParam) {
>        ...
>     }

This is my fault, sorry.  Revisiting the code today I see that I had 
left a method with broken syntax in the class (ironically that I'd added 
to test out this problem); removing this fixed the problem.

Incidentally, I think there is a documentation error in 
http://juneau.apache.org/site/apidocs/overview-summary.html#juneau-rest-server.HttpPartAnnotations.Path

One of the examples suggests 'required' is an allowed attribute of @Path 
but this is not supported in the code:

   // Normal
    @Path(
       name="name",
       description="Pet name",
       required=true,
       example="Doggie"
    )

(I think logically 'required' should not be supported, so it seems a 
documentation rather than code error)

Thanks,

David



> On Thu, Oct 25, 2018 at 2:32 PM David Goddard <goddard@acm.org 
> <mailto:goddard@acm.org>> wrote:
> 
>     Hi,
> 
>     I'm catching up with some of the 7.2.0 changes and I'm hitting some
>     issues updating some pre-7.2.0 code (specifically I'm having problems
>     similar to JUNEAU-85).
> 
>     I note that org.apache.juneau.rest.annotation.Parameter was removed
>     from
>     7.2.0 as part of the Swagger changes (although not all of the
>     documentation seems to have caught up:
>     http://juneau.apache.org/site/apidocs/org/apache/juneau/rest/annotation/MethodSwagger.html#parameters--)
> 
>     So my original code was like:
> 
>         swagger = @MethodSwagger(
>           parameters= {
>             @Parameter(in="path", name="somePathParam", description="xxx"),
>           }
>          ),
> 
>     This would obviously fail in 7.2.0 as @Parameter is absent.
> 
>     However, I'm hitting some issues getting it working with the
>     SimplifiedJson syntax described in
>     http://juneau.apache.org/site/apidocs/org/apache/juneau/rest/annotation/RestMethod.html#swagger--,
> 
>     which is of this format as per the docs:
> 
>         swagger={
>           "parameters:[",
>             "{name:'propertyName',in:'path',description:'The system
>     property
>     name.'},",
>             "{in:'body',description:'The new system property value.'}",
>           "],",
>           "responses:{",
>             "302: {headers:{Location:{description:'The root URL of this
>     resource.'}}},",
>               "403: {description:'User is not an admin.'}",
>            "}"
>          }
> 
>     So my method updates to something like this:
> 
>         @RestMethod(name="GET", path="/foo/{somePathParam}/*",
>     guards=FooGuard.class,
>           swagger = {
>             "parameters:[",
>             
>       "{name:'somePathParam',in:'path',description:'xxx',required:true}",
>             "],"
>           },
>           properties = { // some props }
>         )
>         public ServiceResult someMethod(RestRequest request, RestResponse
>     response, @Path String somePathParam) {
>              // etc.
>         }
> 
>     However, this gives me a syntax error in Eclipse:
> 
>         Type mismatch: cannot convert from String[] to MethodSwagger
> 
>     Any idea what I'm doing wrong here? (I am assuming I'm doing something
>     wrong)
> 
>     Looking at the Pet Store example code, I see a different syntax in
>     PetStoreResource, leading me to change my method to this:
> 
>         @RestMethod(name="GET",
>             path="/foo/{somePathParam}/*",
>             properties ={
>               @Property(name=HTMLDOC_header, value="$L{title}"),
>             },
>             swagger = @MethodSwagger(
>               parameters= {
>                  "{name:'somePathParam',in:'path',description:'xxx'}",
>               }
>             )
>         )
>         public ServiceResult someMethod(RestRequest request,
>                         RestResponse response,
>                         @Path String somePathParam) {
>             return null; //(Not really)
>         }
> 
>     Do we have a documentation issue, or an understanding issue on my part?
> 
>     However, in any case I get a runtime error at the moment:
> 
>     #######
>     Exception occurred while initializing method
>     'some.package.ServiceREST.someMethod'
>     ...
>     Caused by: @Path used without name or value on method
>     'some.package.ServiceREST.someMethod(RestRequest,RestResponse,String)'
>     parameter '2'.
>     #######
> 
>     What am I missing?
> 
>     ....
> 
>     More generally, @Parameter is back in 7.2.1-RC, but it's unclear to me
>     at least what the direction is for this.  Are we now looking to remove
>     these annotations again in a future (major) release, with the favoured
>     approach being the SimplifiedJson syntax?  However, @Parameter is not
>     marked as deprecated in the current RC.  I don't get a clear sense from
>     the docs which way this is going (or indeed where it exactly is now).
> 
>     I'd agree with Gary's sentiment from JUNEAU-85 that 7.1 -> 7.x
>     should be
>     a non-breaking change, but it is the case that having multiple
>     concurrently valid techniques is confusing at present (at least to me);
>     this could be a documentation fix though.
> 
>     Thanks
> 
>     David
> 
> 


Mime
View raw message