Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add backcompat stubs for optional header params #967

Closed
wants to merge 6 commits into from

Conversation

gsheasby
Copy link

Before this PR

Similar to #960 - when an optional Header param is added to a method, no @Deprecated method without the header param is generated. This results in dev breaks, requiring any consumers to add either Optional.empty() or the header, as in this commit.

After this PR

==COMMIT_MSG==
When optional header params are added, Retrofit and Jersey generated code now includes deprecated methods without these headers - similar to those generated when optional query params are added.
==COMMIT_MSG==

Possible downsides?

Still not added for Dialogue - may split this into another PR.

@gsheasby gsheasby requested a review from carterkozak June 22, 2020 13:38
@changelog-app
Copy link

changelog-app bot commented Jun 22, 2020

Generate changelog in changelog/@unreleased

Type

  • Feature
  • Improvement
  • Fix
  • Break
  • Deprecation
  • Manual task
  • Migration

Description

When optional header params are added, Retrofit and Jersey generated code now includes deprecated methods without these headers - similar to those generated when optional query params are added.

Check the box to generate changelog(s)

  • Generate changelog entry

@carterkozak
Copy link
Contributor

I'm worried that defining something along these lines may not compile due to multiple equivalent method signatures:

testOptionalHeaderParam:
  http: POST /test-optional
  args:
    maybeHeaderOne:
      type: optional<string>
      param-type: header
      param-id: MaybeHeaderOne
    maybeHeaderTwo:
      type: optional<string>
      param-type: header
      param-id: MaybeHeaderTwo
    maybeQueryOne:
      type: optional<string>
      param-type: query
    maybeQueryTwo:
      type: optional<string>
      param-type: query

@@ -56,7 +56,7 @@ protected static void validateGeneratorOutput(List<Path> files, Path outputDir,
Files.deleteIfExists(output);
Files.copy(file, output);
}
assertThat(readFromFile(file)).isEqualTo(readFromFile(output));
assertThat(readFromFile(file)).describedAs(output.toString()).isEqualTo(readFromFile(output));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unsure if you're aware, we have a helper function to update the expected output ./gradlew test -Drecreate=true documented here

param-type: query
maybeQuery:
type: optional<rid>
param-type: query
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This example is generated into the suffixed files, but those are never compiled -- it would be helpful to define this somewhere that outputs into a source set that's also compiled for additional verification, otherwise duplicate methods with the same signature won't be caught by CI.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do you have a place in mind where I could put this example?

I think the cases where type clashes may cause the headers to not compile would be exactly when the first query param is optional and has the same type as any header param. This could be detected in code and worked around by not generating backcompat methods for header params if they would clash. It's likely a corner case in practice, but I appreciate that we need something that always compiles.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Adding it to a new service defined in ete-service.yml should provide compilation verification. Using a new service instead of an existing one means you won't need to update any resource implementations to stub the method.

Is there danger in upgrading from an endpoint with an optional header to an endpoint with an optional header and optional query parameter that the same parameter index and type becomes used for another field?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess to be safe you'd want there to always be a non-optional param (of any kind) directly after the last optional header param.

@gsheasby
Copy link
Author

Caught up with @carterkozak via DM. Given the complexities involved in determining whether it's safe to have two dimensions of compatibility backfill methods (headers and query params), we won't be merging this.

@gsheasby gsheasby closed this Jun 25, 2020
@iamdanfox
Copy link
Contributor

iamdanfox commented Jun 25, 2020

@gsheasby is it one specific library that you're concerned about here? Could we explore a local codegen & relocation workflow to make it invincible to any kind of java ABI/API change?

@a-k-g
Copy link

a-k-g commented May 13, 2021

@carterkozak - would something like this work, to avoid the concern of equivalent method signatures:

  1. Parameters in the generated methods would be in the same order as declared in the conjure definition. AuthHeader should always be the first, and any request object should be the last param. So for the example you gave, we should generate
testOptionalHeaderParam(request);
testOptionalHeaderParam(maybeHeaderOne, request);
testOptionalHeaderParam(maybeHeaderOne, maybeHeaderTwo, request);
testOptionalHeaderParam(request, maybeHeaderOne, maybeHeaderTwo, maybeQueryOne, request);
testOptionalHeaderParam(request, maybeHeaderOne, maybeHeaderTwo, maybeQueryOne, maybeQueryTwo, request);
  1. Any time new parameters are added, we enforce (using conjure-backcompat) that they have to be added at the end. checkConjureBackCompat should fail if there is an attempt to add a new parameter in between existing ones.

@policy-bot policy-bot bot requested a review from jkozlowski May 13, 2021 12:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants