Clarify meaning of default value for java annotation properties

[EricWittmann]

Currently this specification includes a set of Java annotations that developers can use to further document their JAX-RS APIs directly in code. All of these annotations have properties (that typically correspond closely with OpenAPI properties) with default values. For example:

@Target({ ElementType.METHOD })
@Retention(RetentionPolicy.RUNTIME)
@Inherited
public @interface Operation {

    /**
     * Provides a brief description of what this operation does.
     *
     * @return a summary of this operation
     **/
    String summary() default "";

    /**
     * A verbose description of the operation behaviour.
     * CommonMark syntax MAY be used for rich text representation.
     *
     * @return a description of this operation
     **/
    String description() default "";

    /**
     * Unique string used to identify the operation. 
     * The id MUST be unique among all operations described in the API.
     * <p>
     * Tools and libraries MAY use the operationId to uniquely identify an operation, 
     * therefore, it is RECOMMENDED to follow common programming naming conventions.
     * </p>
     * @return the ID of this operation
     **/
    String operationId() default "";

Every annotation property has a default value so that developers can pick and choose which properties they wish to include when they apply the operation in their code. For example, when applying the above Operation annotation, the developer can provide a summary without being required to also provide a description and operationId:

@Operation(
    summary = "Get Example"
)
@GET
@Path("/example")
public String getExample();

The Microprofile OpenAPI implementation's annotation scanner is responsible for turning all of these annotations into a generated OpenAPI document.

The default values for all properties in all MP+OAI annotations are meant to indicate to the annotation scanner that the developer did not include that property in their application of the annotation. The default values are NOT meant to indicate what value the property will have in the generated OpenAPI document.

For the example above, this is what the OpenAPI annotation scanner might generate:

{
  "openapi": "3.0.2",
  "paths": {
    "/example": {
      "get": {
        "summary": "Get Example",
        "responses": {
          "200": {
            "content": {
              "application/json": {
                "schema": {
                  "type": "string"
                }
              }
            }
          }
        }
      }
    }
  }

Notice that no operationId is included, and no description is included in the output, despite the @Operation annotation being applied. That annotation has both operationId and description properties with default value of empty string. Nevertheless, the annotation processor did not generate those properties in the output document. Again, the reason for this is that the default values for annotation properties are intended to signal to the annotation processor that the developer did not include the property when they added the annotation to their code, so the annotation processor should do whatever is appropriate for that property. In most cases, the appropriate thing is to omit the property from the output.

For some properties this behavior is sensible, but for others it is more confusing. However, because there is no way, in Java, to set the default value of an annotation property to null, it's the best we've got.

So to clarify the behavior of these annotations, we should consider updating the javadoc for at least the annotation properties with confusing default values. This is a rather subjective task, so I think we can start by enumerating which of the properties might be confusing by listing them in comments to this issue.

[tjquinno]

I would suggest adding a note about how the defaulted values are to be represented in the internal model. That model and the representation of defaulted values is part of the contract with any code that consumes the model.

The spec should require either that

1. no value in the internal model of the app's API corresponding to annotation properties will be null, or
2. any consumer of the internal model (such as filters) should be prepared for such parts of the model to be null and should handle nulls as indicating defaulted values.

I claim choice 1 makes more sense because, as Eric's write-up points out, the Java language provides no mechanism to assign a null to an annotation property -- by either default or explicit assignment.

Background:

Eric and I had been discussing some aspects of this in a private email thread earlier. What triggered our conversation was this:

I had incorrectly set up how I ran the TCK with SmallRye. I ended up inadvertently using a filter from one set of tests in a second set of tests. In the second set was an endpoint with a @Content annotation with no explicit schema value.

The filter in question deferences the content's schema's description without checking for a null schema first. This seems OK to me, given that there's no way to specify a null value for an annotation property.

Yet the SmallRye annotation processor did set the content's schema to null in the internal model. When passed that model element, the filter threw an NPE trying to dereference the content's null schema.

Granted, I triggered the NPE by setting the TCK up incorrectly. But that just highlighted that the contract among MP OpenAPI, the annotation processor, and the filter (or any other code that consumes the internal model) is not clear on this point. It would be good if the spec nailed this down one way or the other.

[arthurdm]

Thanks for the well explained summary @EricWittmann

I think this pattern works well when the value is the equivalent of null, such as Schema() or "", but could be problematic for booleans and integers.

For example: a boolean property with default of false - does the processor know that it is getting the value of false from the default and therefore it can omit this entry from the generated OAS3 doc versus the user actually setting this value to be false - in which case it should be included in the generated OAS3 doc?

I think where I am getting at is that perhaps we need to specify that the processor MUST (?) detect if a value is set using other means - such as how Smallrye does it.

This way the processor can easily ignore unspecified attributes, with the exception of special cases, such as @Parameter.schema, for which we would add Javadoc describing what happens when this value is not specified.

[EricWittmann]

OK so while I understand Tim's position on this (he's advocating for his option 1 above) I disagree. If we implemented option 1 above, the generated OpenAPI documents would be a big mess. Take @Operation as just one example off the top of my head. If you apply that annotation in your code just so you can supply a summary and description, it will look like this:

@Operation(
    summary = "Get Example",
    description = "This is the description of my operation."
)

The result is that the generator will add both summary and description to the OpenAPI operation. Something like:

"get" : {
  "summary": "Get Example",
  "description": "This is the description of my operation."
}

If we require that the annotation processor never allow null values in the model, then instead we'll get this:

"get" : {
  "summary": "Get Example",
  "description": "This is the description of my operation.",
  "operationId": "",
  "deprecated": false,
  "hidden": false
}

I have two problems with this specific example. First is that we've added properties that aren't needed and weren't set in the annotation by the user (the whole point of all this) and also that we've very likely added an invalid operationId to the generated output (empty string is not a valid operationId). If the developer wants valid OpenAPI, they will need to put an operationId into their @Operation annotation.

OK but maybe you're saying that's actually not that bad. We can probably address the operationId problem via some javadoc and/or update the spec.

If you look at another annotation, the problem gets WAY worse. Namely the @Schema annotation, which has 37 properties. In that case, just applying the annotation to your java bean so you can set a title would cause all 36 other properties to also be set in the generated model. And some of those have very specific meanings (e.g. multipleOf or maxLength). The absence of a value for those properties is meaningful. Requiring them to be set would be bad.

IMO :)

[EricWittmann]

In response to @arthurdm - I agree that boolean is probably the most problematic, although some of the numeric values are also a bit wonky.

I'm not sure about requiring a SmallRye style approach for the processor. That might be a bit of a tough requirement for some impls...

[tjquinno]

Looks like I need to clarify.

I heartily agree with the goal of a minimalist output document. I'm not advocating for showing values in the document that are defaulted.

My main point is that the spec needs to be explicit about whether nulls can appear in the internal model because, in my view, that would be surprising given that annotation properties cannot be declared with null values, explicit or default. That internal model is populated by annotation processors and developer-provided model readers and consumed by developer-provided filters and the document writer. All parties need to know what the rules are, so the spec has to tell them.

My secondary point is that I see the pros and cons of using nulls in the internal model to represent "not specified." Because there's no way to declare an annotation property to have a null value, I tend to think it's potentially astonishing to a filter developer, for example, to need to do null checks on the corresponding fields in the model. That said, it's certainly convenient in other places to use nulls this way.

In truth, the second point is not a hill I'm willing to die on. The first point, though, I think is. The spec has to say whether nulls are OK and what that means.

[EricWittmann]

OK thanks for the clarification. I suspect we can agree that nulls in a data model is a pretty normal thing to see. However, your point (if I finally understand it fully) is that nulls in the data model produced by the annotation scanner seems bad because the properties in the java annotations cannot be null.

Now, the latter is only true because Java is stupid about that - but it does remain true.

It's worth pointing out that filtering is the final processing step, so it will be filtering a model that was produced through some combination of:

• Model reader
• Static file
• Annotations

Even if we force non-nulls into the data model for the 3rd step, it seems to me that nulls are going to get in there for the first 2. So filtering needs to handle nulls.

In any case, I understand your main point: the spec should be explicit about whether null values can be in the data model. I don't see why we couldn't add some text to the spec for that. I probably don't see it as a priority the way that you do, but I also don't have any objection. :)

If you're interested in submitting a PR as a suggestion of this change, that'd be awesome. If not that's fine too - we'll discuss on the next call.

[tjquinno]

Indeed, in general nulls in a data model are normal. We do have the data point that the filter in the TCK was not written to tolerate nulls. https://github.com/eclipse/microprofile-open-api/blob/2.0/tck/src/main/java/org/eclipse/microprofile/openapi/filter/AirlinesOASFilter.java#L132 Granted, it was probably designed to be fine-tuned to the specific endpoints it was expecting. But developers might well use TCK code as an example so that code should reflect the possibility of nulls.

[EricWittmann]

This has been a very interesting discussion, and also relates to the discussion about request bodies here: #349

We discussed this again during today's hangout and have reached some conclusions.

The first is that after considering all the points that have been made, we still think that the current way annotation property defaults are treated is appropriate and the least evil of the options we have. So we're going to continue with the approach that default values for the MP+OAI annotation properties should be treated (by implementations) as though the property was not specified by the developer. This is not perfect but we think it's the best developer experience.

However, we can (and will) make some changes to make things clearer for both implementer and users of the spec. Things we are going to change/add:

• Update the TCK so that the filter checks for nulls appropriately
• Document the treatment of annotation default values in the text of the specification

Now, typically the default values for annotation properties should match (as closely as possible) the default value for that property in the upstream OpenAPI specification. This should cut down on some confusion, but is not always possible and more confusingly is not always desirable. Because the annotation scanner in Microprofile+OpenAPI is targeting JAX-RS, we feel that default scanning behavior should accurately reflect the characteristics of JAX-RS rather than the default values of OpenAPI. So in some exceptional cases, the default value for certain annotation properties may differ from the upstream specification default so that the default behavior of the annotation scanner more closely models the actual behavior of the application.

The current example of this is #349, where we will be changing the default value of required for request bodies because request bodies are by default required by JAX-RS. In these exceptional cases, we will be documenting the reason why we are diverging from the typical behavior in two places:

• In the Javadoc of the annotation property itself
• In a table somewhere in the specification (probably in the spec wiki, not the text of the spec)
• In the TCK by adding a test specifically for the exceptional case

Hopefully this is a rational choice we are making, given the lack of completely ideal choices. If Java would only allow null as a default value for an annotation property, things would be much easier!

[jskov-jyskebank-dk]

This may be a silly idea, but would it make sense to do something like this:

String operationId() default "__method_name__";

The generator should substitute "__method_name__" with name of the resource method name.

For extra points, let a new OpenApi flag control whether to default substitution to empty string (as it happens today), or resource method name.

This would allow generation of richer APIs without having to annotate all methods, as we have to do today. Which is a pain.