feat: support deprecated field for OpenAPI parameters

[lachlancooper]

References to other Issues or PRs

Fixes #2906

Have you read the Contributing Guidelines?

Brief description of what is fixed or changed

The protoc-gen-openapiv2 generator now inspects each proto field’s deprecated option when building query/path parameters, and if set, writes the OpenAPI deprecated flag on the corresponding parameter object. This allows the proto-level deprecation signal to flow straight into the emitted openapiv2 document.

Other comments

[johanbrandhorst]

Was this all it took 😂? I ran the CI jobs, lets see if there are any failures. In any case, could you try applying a "deprecated" option to one of our protobuf fields to check that it works? Also, could we try to add an annotation option in addition to the protobuf option? It would be nice if users could deprecate OpenAPI fields independently of the protobuf deprecation. See CONTRIBUTING.md for steps on how to regenerate our test files.

[lachlancooper]

Was this all it took 😂?

Surprisingly, apparently so? 😆 This has been bugging me again lately, so rather than trying to add a consistent marker to all our field comments manually I decided to try fixing it properly.

In any case, could you try applying a "deprecated" option to one of our protobuf fields to check that it works?

While working on this I tested adding a new deprecated field to a_bit_of_everything.proto, and the generated json looks good:

  string should_be_deprecated = 52 [deprecated = true];

{
  "name": "shouldBeDeprecated",
  "in": "query",
  "required": false,
  "deprecated": true,
  "type": "string"
}

Should I make this change in the PR?

Also, could we try to add an annotation option in addition to the protobuf option?

Makes sense, let me give this a go.

[lachlancooper]

I've implemented a deprecated annotation option under field_configuration. I've also updated the docs to cover the two different ways to mark a parameter as deprecated.

Since this whole PR is effectively a breaking change, should this all be behind a new plugin option, similar to enable_rpc_deprecation?

[johanbrandhorst]

I agree that it is a breaking change, but there's also a case for this being a "bug fix". If someone had their protobuf fields marked deprecated and it didn't show up in their OpenAPI specs, that could be intentional, but it could also be an unfortunate shortcoming. As long as we make it easy for users to discover how to revert back to the old behavior (e.g., set the openapi field configuration deprecation option to false), it might be OK? What do you think? Are we more likely to delight users or annoy users with this?

[lachlancooper]

As long as we make it easy for users to discover how to revert back to the old behavior (e.g., set the openapi field configuration deprecation option to false)

As it stands right now, this won't be possible. The new field config option is a simple bool without presence, so if it's not explicitly set to true, then it defaults to false and we can't distinguish that from an explicit false.

In other words, the logic between the annotation and the proto option is currently OR, but this means a (arguably) breaking change with no way for users to override it. But if we make the logic AND, then users would need to add the annotation to every single field they want to deprecate, which is obviously not sensible.

I guess this is an argument in favour of adding a new top-level plugin option?

[johanbrandhorst]

I see, thanks for the clarification. So we allow the user to specify whether a field is deprecated in OpenAPI or in both OpenAPI and protobuf. We'll definitely want an option for the protobuf option, since that would be a breaking change with no way to disable it without un-deprecating the protobuf field, but maybe since the OpenAPI annotation is new and defaults to off we don't need the option to apply to that? I could see it being confusing for users that they have to set the field option and the generator option to use the OpenAPI option. What do you think?

[lachlancooper]

I agree that makes sense. So ultimately we'll have three toggles here:

A: deprecated proto field option (existing)
B: deprecated OpenAPI annotation (new)
C: enable_field_deprecation plugin option (new)

And for each OpenAPI parameter in the output, it will be marked deprecated based on this logic:

(A && C) || B

Does this seem reasonable?

[johanbrandhorst]

[johanbrandhorst]

This is an excellent contribution, thank you so much for taking the time!