fix: required properties of message type are required in OpenAPI

[gostajonasson]

Properties of message type having the annotation [(google.api.field_behavior) = REQUIRED] are not marked as required in the generated OpenAPI specification.
This PR fixes that.

A maintainer should sanity check this PR since it naively removes logic someone added on purpose linking to a "JSON Reference syntax".

Fixes #2837

[google-cla]

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

[keyz]

Would love to see this merged!

@gostajonasson -- I added a few tests (in my fork) to verify your change, and I think you have the right fix.

• keyz@91060e6: before applying the fix, required annotations are missing
• keyz@deab66e: after applying the fix, openapi files seems legit with no regressions

Feel free to copy anything from my fork if it helps to get your PR merged :)

[johanbrandhorst]

Huh, this is a surprisingly small fix. I can't say I know why that check was there. @MakDon what do you think about the changes? You've contributed a fix in this space recently.

[MakDon]

Huh, this is a surprisingly small fix. I can't say I know why that check was there. @MakDon what do you think about the changes? You've contributed a fix in this space recently.

I will take a look at it ASAP :D

[gostajonasson]

Thanks @keyz!
@johanbrandhorst @MakDon just tell me if you want me to add the tests from @keyz to this PR.

[MakDon]

It seems that this solution would cause another issue, see #1937 AND #1944
I am trying to confirm if this PR would break the generation again.

IMO, it is better to add the tests from @keyz
I am trying to confirm why the test TestSchemaOfField is not broken when running the CI

[johanbrandhorst]

More tests shouldn't hurt, so please feel free to add them here. Thanks @MakDon for taking a look!

[gostajonasson]

It seems that this solution would cause another issue, see #1937 AND #1944

Yes you are correct @MakDon, this PR is naive and I need help fixing it.
This PR would cause message types marked with the OUTPUT_ONLY field behaviour as read only like this:

  Location pickup_location = 1 [(google.api.field_behavior) = OUTPUT_ONLY];

    properties:
      pickupLocation:
        $ref: '#/definitions/Location'
        readOnly: true

Which isn't recommended since other members than $ref should be ignored per the JSON Reference spec.

But I'm also seeing that the description field is added (somewhere else in the code) next to $ref members so that's an existing bug.

Personally I think that the best fix is outside of the schemaOfField function and not output any members next to $ref (if openapiSchemaObject.Ref is set) in the final OpenAPI spec.
What I mean is that the schemaOfField function should not be responsible for filtering out fields just because it is of message type. That should a renderer do since it's OK to list the required fields outside of the property:

  Shipment:
    type: object
    properties:
      pickupLocation:
        $ref: '#/definitions/Location'
    description: A description.
    required:
      - pickupLocation

[gostajonasson]

Pushed a fix for the bug that @MakDon mentioned my initial fix would cause.
Right now this PR fixes all bugs for my use case.
Will add the tests from @keyz later.

[johanbrandhorst]

Hi @gostajonasson, thanks again for working on this, do you think you'll bring this over the line in the next week or so? I'm just planning out the next release timing. Thanks!

[gostajonasson]

Hi @gostajonasson, thanks again for working on this, do you think you'll bring this over the line in the next week or so? I'm just planning out the next release timing. Thanks!

@johanbrandhorst Yes I hope so! Sorry for the delay.

[gostajonasson]

@keyz Do you think I should add everything from the two commits you linked to?
I'm not familiar with this repo and don't know exactly what to copy to test the fix in this PR.

[keyz]

@gostajonasson you can:

1. start with copying what I did in examples/internal/proto/examplepb/a_bit_of_everything.proto (commit keyz@91060e6).
2. after that, follow CONTRIBUTING.md to regenerate the fixtures
3. you might need to tweak examples/internal/clients/abe/BUILD.bazel, but my commits (+ keyz@deab66e) should contain everything you need

[gostajonasson]

@keyz Thanks for the instructions! I've followed them and pushed the changes. Happy for a review and a workflow run. 🤗

[johanbrandhorst]

Thanks for your contribution!

[johanbrandhorst]

https://github.com/grpc-ecosystem/grpc-gateway/releases/tag/v2.12.0 has been released