palantir / palantir/conjure-java

Overloaded methods for optional query params may silently allow compilation in some unsafe cases

Open
#47 0 comments 0 reactions 0 assignees View on GitHub

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

Open the contributing guide

First steps

  1. Read the whole issue, then the project's contributing guide.
  2. Comment on the issue to say you are picking it up — it saves two people doing the same work.
  3. Fork the repository and make your change on a branch.
  4. 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

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.