conjure-java icon indicating copy to clipboard operation
conjure-java copied to clipboard

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

Open a-k-g opened this issue 7 years ago • 0 comments

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.

a-k-g avatar Jul 18 '18 16:07 a-k-g