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
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 7 additions & 0 deletions changelog/@unreleased/pr-967.v2.yml
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
type: improvement
improvement:
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.
links:
- https://github.com/palantir/conjure-java/pull/967
Original file line number Diff line number Diff line change
Expand Up @@ -186,19 +186,65 @@ private MethodSpec generateServiceMethod(
/** Provides a linear expansion of optional query arguments to improve Java back-compat. */
private List<MethodSpec> generateCompatibilityBackfillServiceMethods(
EndpointDefinition endpointDef, TypeMapper returnTypeMapper, TypeMapper argumentTypeMapper) {
List<ArgumentDefinition> headerArgs = new ArrayList<>();
List<ArgumentDefinition> queryArgs = new ArrayList<>();

for (ArgumentDefinition arg : endpointDef.getArgs()) {
if (arg.getParamType().accept(ParameterTypeVisitor.IS_HEADER)
&& arg.getType().accept(DefaultableTypeVisitor.INSTANCE)) {
headerArgs.add(arg);
}
if (arg.getParamType().accept(ParameterTypeVisitor.IS_QUERY)
&& arg.getType().accept(DefaultableTypeVisitor.INSTANCE)) {
queryArgs.add(arg);
}
}

if (!headerArgs.isEmpty()) {
// ensure the correct ordering of parameters by creating the complete sorted parameter list
List<ParameterSpec> sortedParams = createServiceMethodParameters(endpointDef, argumentTypeMapper, false);
List<ArgumentDefinition> sortedArgs = sortedParams.stream()
.map(param -> endpointDef.getArgs().stream()
.filter(arg -> arg.getArgName().get().equals(param.name))
.findFirst())
.filter(Optional::isPresent)
.map(Optional::get)
.collect(Collectors.toList());

Optional<ArgumentDefinition> firstParameterAfterLastHeader = Optional.empty();
for (int i = 0; i < sortedArgs.size(); i++) {
ArgumentDefinition arg = sortedArgs.get(i);
boolean isOptionalHeaderArg = arg.getParamType().accept(ParameterTypeVisitor.IS_HEADER)
&& arg.getType().accept(DefaultableTypeVisitor.INSTANCE);
if (isOptionalHeaderArg) {
if (i < (sortedArgs.size() - 1)) {
firstParameterAfterLastHeader = Optional.of(sortedArgs.get(i + 1));
} else {
firstParameterAfterLastHeader = Optional.empty();
}
}
}

if (!firstParameterAfterLastHeader.isPresent()
|| firstParameterAfterLastHeader.get().getType().accept(DefaultableTypeVisitor.INSTANCE)) {
// The last optional header is not followed by a non-optional parameter, so we can't guarantee
// uniqueness of signatures if we generate backfill methods for headers.
headerArgs = ImmutableList.of();
}
}

List<MethodSpec> alternateMethods = new ArrayList<>();
for (int i = 0; i < queryArgs.size(); i++) {
alternateMethods.add(createCompatibilityBackfillMethod(
endpointDef, returnTypeMapper, argumentTypeMapper, queryArgs.subList(i, queryArgs.size())));
for (int j = 0; j <= headerArgs.size(); j++) {
for (int i = 0; i <= queryArgs.size(); i++) {
List<ArgumentDefinition> extraHeaderArgs = headerArgs.subList(j, headerArgs.size());
List<ArgumentDefinition> extraQueryArgs = queryArgs.subList(i, queryArgs.size());
List<ArgumentDefinition> extraArgs = new ArrayList<>(extraHeaderArgs);
extraArgs.addAll(extraQueryArgs);
if (!extraArgs.isEmpty()) {
alternateMethods.add(createCompatibilityBackfillMethod(
endpointDef, returnTypeMapper, argumentTypeMapper, extraArgs));
}
}
}

return alternateMethods;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -201,23 +201,32 @@ private HttpPath replaceEncodedPathArgs(HttpPath httpPath) {
private List<MethodSpec> generateCompatibilityBackfillServiceMethods(
EndpointDefinition endpointDef, TypeMapper returnTypeMapper, TypeMapper argumentTypeMapper) {
Set<ArgumentName> encodedPathArgs = extractEncodedPathArgs(endpointDef.getHttpPath());
List<ArgumentDefinition> headerArgs = new ArrayList<>();
List<ArgumentDefinition> queryArgs = new ArrayList<>();

for (ArgumentDefinition arg : endpointDef.getArgs()) {
if (arg.getParamType().accept(ParameterTypeVisitor.IS_HEADER)
&& arg.getType().accept(DefaultableTypeVisitor.INSTANCE)) {
headerArgs.add(arg);
}
if (arg.getParamType().accept(ParameterTypeVisitor.IS_QUERY)
&& arg.getType().accept(DefaultableTypeVisitor.INSTANCE)) {
queryArgs.add(arg);
}
}

List<MethodSpec> alternateMethods = new ArrayList<>();
for (int i = 0; i < queryArgs.size(); i++) {
alternateMethods.add(createCompatibilityBackfillMethod(
endpointDef,
returnTypeMapper,
argumentTypeMapper,
encodedPathArgs,
queryArgs.subList(i, queryArgs.size())));
for (int j = 0; j <= headerArgs.size(); j++) {
for (int i = 0; i <= queryArgs.size(); i++) {
List<ArgumentDefinition> extraHeaderArgs = headerArgs.subList(j, headerArgs.size());
List<ArgumentDefinition> extraQueryArgs = queryArgs.subList(i, queryArgs.size());
List<ArgumentDefinition> extraArgs = new ArrayList<>(extraHeaderArgs);
extraArgs.addAll(extraQueryArgs);
if (!extraArgs.isEmpty()) {
alternateMethods.add(createCompatibilityBackfillMethod(
endpointDef, returnTypeMapper, argumentTypeMapper, encodedPathArgs, extraArgs));
}
}
}

return alternateMethods;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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

}
}
}
15 changes: 15 additions & 0 deletions conjure-java-core/src/test/resources/example-service.yml
Original file line number Diff line number Diff line change
Expand Up @@ -224,6 +224,21 @@ services:
type: optional<rid>
param-type: query

testOptionalHeaderParam:
http: POST /test-optional-header-param
args:
query: string
maybeHeader:
type: optional<string>
param-type: header
param-id: MaybeHeader
implicit:
type: rid
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.


testBoolean:
http: GET /boolean
returns: boolean
Expand Down

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Loading