palantir / palantir/conjure-java
Overloaded methods for optional query params may silently allow compilation in some unsafe cases
Nobody has claimed this yet.
- Dominant language
- Java
- Stars
- 39
- Forks
- 49
- Avg merge
- 8h 22m
- Merged PRs (30d)
- 32
Description
With #21, we generate @Deprecated methods with the intention of avoiding compile breaks in cases where new query params are added. However, in some cases we might silently let compilation go through when it shouldn't. Specifically, if we add another optional query params with the same type as an existing one and in the api.yml declare it right before the existing one.
As an example, lets say we have the following API:
getTransactionsPage:
http: POST /get-page
args:
query: string
optionalEnd:
type: optional<ResourceIdentifier>
param-type: query
returns: set<ResourceIdentifier>
-----
@POST
@Path("catalog/get-page")
Set<ResourceIdentifier> getTransactionsPage(
@HeaderParam("Authorization") AuthHeader authHeader,
@QueryParam("optionalEnd") Optional<ResourceIdentifier> optionalEnd,
String query);
@Deprecated
default Set<ResourceIdentifier> getTransactionsPage(AuthHeader authHeader, String query) {
return getTransactionsPage(authHeader, Optional.empty(), query);
}
and this is being used as follows in the client code:
Set<ResourceIdentifier> transactions = getTransactionsPage(authHeader, endTransactionRid, query);
Lets say the API was included to also have an optionalStart query param:
getTransactionsPage:
http: POST /get-page
args:
query: string
optionalStart:
type: optional<ResourceIdentifier>
param-type: query
optionalEnd:
type: optional<ResourceIdentifier>
param-type: query
returns: set<ResourceIdentifier>
-----
@POST
@Path("catalog/get-page")
Set<ResourceIdentifier> getTransactionsPage(
@HeaderParam("Authorization") AuthHeader authHeader,
@QueryParam("optionalStart") Optional<ResourceIdentifier> optionalStart,
@QueryParam("optionalEnd") Optional<ResourceIdentifier> optionalEnd,
String query);
@Deprecated
default Set<ResourceIdentifier> getTransactionsPage(AuthHeader authHeader, String query) {
return getTransactionsPage(authHeader, Optional.empty(), Optional.empty(), query);
}
@Deprecated
default Set<ResourceIdentifier> getTransactionsPage(
AuthHeader authHeader, Optional<ResourceIdentifier> optionalStart, String query) {
return getTransactionsPage(authHeader, optionalStart, Optional.empty(), query);
}
Compilation of the client code wouldn't break but it is now doing something semantically different.
To prevent, we'd have to ensure that whenever devs add new optional query params they always declare it last. However, I don't think thats something we do at the moment.
I discussed this @iamdanfox and he had a proposal around using builders instead to pass optional query and header params.
Contributor guide
First steps
- Read the whole issue, then the project's contributing guide.
- Comment on the issue to say you are picking it up — it saves two people doing the same work.
- Fork the repository and make your change on a branch.
- Open a pull request that references the issue number.
Research direction
Start with the deprecated-method generation introduced by #21 and the api.yml argument ordering shown in the examples. Compare the generated overloads before and after adding optionalStart, then review the builder proposal discussed with iamdanfox. Done means adding a same-typed optional query parameter cannot silently change existing client behavior.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- java
- Domain
- api, tooling
- Issue type
- Bug
- Difficulty
- 5/5
- Estimated time
- Over a week
- Activity status
- Stale
- Clarity
- Mostly clear
- Newbie friendliness
- 35/100