Better error message for parameters

[phillip-kruger]

see quarkusio/quarkus#26848

[phillip-kruger]

cc @cowtowncoder

[phillip-kruger]

@MikeEdgar - Basically we should be able to determine if we could not apply the default in parameter (because of no JAX-RS or ever Spring annotation) and provide a better error message ? W.d.y.t ?

[MikeEdgar]

Yeah, that can be done. At the moment, it will need to happen after all parameters for the method are processed ( since we support the annotations being placed in different locations, even for the same parameter ).

I'll look more closely at the stack trace in the Quarkus issue to see if there is anything that can make the handling of this more elegant.

[phillip-kruger]

Yes I guess ever after that static schema documents (if provided) is merged in ?

[phillip-kruger]

.. and all filters has ran ?

[MikeEdgar]

Post processing validation and warnings would certainly be nice. For this case we can also make some determination on the validity of just the annotations.

Aside, I've been thinking for the next major release, the reader and static input is made available to the scanner to make better informed decisions on defaults and warnings like we're proposing here. That's a much bigger topic though 😃

[phillip-kruger]

Yup :)

[cowtowncoder]

This all sounds good. My starting was just that ANY indication of how to resolve the issue (in my case by just adding @PathParam) would be welcome, instead of

java.lang.RuntimeException: io.quarkus.builder.BuildException: Build failure: Build failed due to errors
        [error]: Build step io.quarkus.smallrye.openapi.deployment.SmallRyeOpenApiProcessor#build threw an exception: java.lang.NullPointerException: Cannot invoke "org.eclipse.microprofile.openapi.models.parameters.Parameter$In.equals(Object)" because the return value of "org.eclipse.microprofile.openapi.models.parameters.Parameter.getIn()" is null
        at io.smallrye.openapi.runtime.scanner.spi.AnnotationScanner.isPathParameter(AnnotationScanner.java:944)
        at io.smallrye.openapi.runtime.scanner.spi.AnnotationScanner.lambda$getRequestBodyParameterClassType$7(AnnotationScanner.java:930)

but I realize that asides from changing exception itself from NPE (which is probably easy), metadata about source location etc may not be easily accessible at point where problem is found.

Although in hindsight, that Parameter$In.equals(... does hint at where to look at... when one knows what to look for.

[phillip-kruger]

@MikeEdgar - also look at the last comment in the Quarkus issue. There is a parameter in the Application level but it does not seem to filter though. I am not sure how that is supposed to work. Do you know ?

@cowtowncoder the open api code (annotations and other ways) does not change the JAX-RS (or Spring) behavior. It just create the schema doc. So in you example you will till (even with parameter in have an issue as the JAX-RS is expecting a raw value in the request body. You need to annotation the JAX-RS code with @QueryParam anyway.

[tatu-at-datastax]

@phillip-kruger That makes sense. I was not (and am not) claiming usage as-is is correct, just that the error handling/reporting is not working well. That JAX-RS vs OpenApi need different annotations makes sense and I was guessing might be the case, esp. for JAX-RS side (OpenApi handler can perhaps deduce more information).

[MikeEdgar]

There is a parameter in the Application level but it does not seem to filter though. I am not sure how that is supposed to work.

This should render as a $ref in the output, with the parameter information placed under components (like a referenced schema).

[phillip-kruger]

@MikeEdgar - I don't think that worked ... At least not in the example provided.

@tatu-at-datastax Thanks for the report. I also feel that OpenApi can deduce more info. We always try and make the default as good as possible. Do you have any suggestions for this (or any other) case ?

[MikeEdgar]

I noticed the class with @OpenAPIDefinition in the example does not extend jakarta.ws.rs.core.Application. Here's the "correct" output (aside from the requestBody being there)...

{
  "openapi" : "3.0.3",
  "info" : {
    "title" : "",
    "version" : ""
  },
  "tags" : [ {
    "name" : "data",
    "description" : "DML operations"
  } ],
  "paths" : {
    "/hello" : {
      "get" : {
        "tags" : [ "Xdata" ],
        "summary" : "Get a doc",
        "description" : "Get a document from data store",
        "parameters" : [ {
          "name" : "sort",
          "in" : "query",
          "description" : "Keys to sort by",
          "schema" : {
            "type" : "string"
          }
        }, {
          "$ref" : "#/components/parameters/raw"
        } ],
        "requestBody" : {
          "content" : {
            "application/json" : {
              "schema" : {
                "type" : "boolean"
              }
            }
          }
        },
        "responses" : {
          "200" : {
            "description" : "OK",
            "content" : {
              "application/json" : {
                "schema" : {
                  "type" : "string"
                }
              },
              "*/*" : {
                "schema" : {
                  "type" : "string"
                }
              }
            }
          },
          "400" : {
            "$ref" : "#/components/responses/GENERAL_400"
          }
        }
      }
    }
  },
  "components" : {
    "responses" : {
      "GENERAL_400" : {
        "description" : "Bad request"
      }
    },
    "parameters" : {
      "raw" : {
        "name" : "raw",
        "in" : "query",
        "description" : "Whether to 'unwrap' results object (omit wrapper)",
        "schema" : {
          "type" : "boolean"
        }
      }
    }
  }
}

[MikeEdgar]

Also, in the above case, the scanner would log the following. Let me know if this could be reworded to be more clear, etc.

WARN: SROAP07902: Unable to associate MicroProfile `@Parameter` with framework parameter. Method: jakarta.ws.rs.core.Response getDocument(boolean, java.lang.String), Parameter.Name: raw, Parameter.In: null

[phillip-kruger]

Cool, I missed that. So that all works as expected. I also missed that warning ! That is actually what we want right ? @cowtowncoder ? I wonder if that should rather have been an error ?

[MikeEdgar]

That output and the log message are all in my local changes only :-) I was thinking warning because the scanner can keep going. Just a way to signal that the output is likely not what is desired.

[phillip-kruger]

O, ok. Yea - failing that situation is a breaking change. Maybe the warning is ok. @cowtowncoder any thoughts ?

[cowtowncoder]

Ok so adding WARN would improve things a lot, although perhaps error would make more sense.
In my case error would not be a breaking change (since build already fails) but if there are other cases then yes, backwards compatibility is an important consideration.
But I trust that you two have much better understanding of the big picture so I trust your judgment here.

Also: good point about missing Application -- I copied an existing baseline which didn't have it either; will fix that.