Java Template: Make ApiInvoker more pluggable

[karussell]

We needed simple key authentication and with this change you can easily extend the ApiInvoker and e.g. hook into ApiInvoker.invokeAPI to modify the queryParams. And for header based authentication you could change the headers there too.

Example usage:

ApiInvoker myInvoker = new ApiInvoker() {
  public String invokeAPI(String host, String path, String method, Map<String, String> queryParams, ...) {
    queryParams.put("key", someKey);
    super.invokeAPI(host, path, method, queryParams, ...);
  }
}

SomeApi api = new SomeApi(myInvoker);

[karussell]

Just to clarify this: the future will have something like apiInvoker.setAutorization(someAPIKeyAuth) but this is just not yet done in the Java client, right?

And why is there a separate Android client? Maybe both should just use okhttp and they will work fine and could share 100% code.

[frodrigo]

I was more think to something like
public void addDefaultHeader(String key, String value) {
defaultHeaderMap.put(key, value);
}
but for queryParams :
public void addDefaultQueryParams(String key, String value) {}

[fehguy]

@wing328 what do you think of this? It does solve the static issue

[wing328]

@fehguy if you've time, I would recommend you to take a look at the PR #765 [PHP] Add authentication support (API key, HTTP basic)

The approach I used is a Configuration class to store authentication related setting. The benefit is that

1. the developers only need to set it once and ONLY methods that require authentication would get the proper setting (header or query parameters)
2. IMO setting the authentication setting (API key, username or password) in the Configuration class is more "natural" than doing it in the apiInvoker
3. Configuration can be further extend in the future (e.g. a switch to enable logging)

If this approach looks good, we can extend this to other languages (Java, Ruby, Python, C#, etc)

[wing328]

FYI. Here is what the code (PHP) looks like:

    // initialize the API client without host
    $api_client = new SwaggerClient\APIClient();
    SwaggerClient\Configuration::$apiKey['api_key'] = '111222333444555';
    $pet_id = 10005;  // ID of pet that needs to be fetched     
    $pet_api = new SwaggerClient\PetAPI($api_client);
    // return Pet (model)
    $response = $pet_api->getPetById($pet_id);

getPetById will automatically add api_key: 111222333444555 to the header

[karussell]

Well for me this looks very similar as I need to pass the config to every API like I would need to do it with the ApiInvoker too:

   ApiInvoker iv = new ApiInvoker();
   PetApi vrpApi = new PetApi(iv);

[wing328]

@karussell I believe your change aims to allow different apiInvoker instances to have different set of default headers.

Is your goal to support authentication using default headers ? What if not all methods (e.g. within PetApi) require authentication ?

The solution I proposed above will only apply the authentication setting based on the Swagger spec.

[karussell]

The intend is not really to allow different apiInvokers but to make it possible in the Java world to intercept every call.

What if not all methods (e.g. within PetApi) require authentication ?

It is up to you as you can decide on demand which header you add for which method.

[wing328]

I agree your proposed change would make ApiInvoker more pluggable (compared with current approach that uses a static ApiInvoker).

Consider the following code:

   PetApi vrpApi = new PetApi();
   UserApi userApi = new UserApi();

Can both PetApi and UserApi share the same default apiInvoker instance (created automatically if no argument passes to the constructor)? Otherwise existing code using the Java client would break if I'm not mistaken.

(Personally I would recommend not to use default header or query parameter for authentication)

[karussell]

Otherwise existing code using the Java client would break if I'm not mistaken

Yes, the will break. but using static objects hidden behind the scene is not the way to go for the future IMO

(Personally I would recommend not to use default header or query parameter for authentication)

Why? Or what do you recommend?

[wing328]

For the REST API SDKs I used before, none of them would require me to add a default header/query parameter for authentication. Usually the API key or other authentication setting is passed when constructing an object or via a configuration block and the SDK should be smart enough to tell which methods require the authentication parameter (header/query).

[karussell]

Now I know what you mean. This change here would enable such a convenience, it is not an "api key implementation", just making it possible.

[karussell]

To avoid confusion: the clients won't break e.g. when releasing the java module, because you would need to specify the new version where the developers then can easily change the minor code to inject the apiInvoker

[wing328]

Yes, the will break. but using static objects hidden behind the scene is not the way to go for the future IMO

From a developer perspective, I would prefer the proposed change to be as transparent as possible since I do not have a need to customize apiInvoker as the default one is good enough for me.

In other words, it would be nice if your proposed change is backward compatible so that developers do not need to update their code to make it work.

[karussell]

You cannot make the ApiInvoker with static pluggable, really :)

(If you really wanted you could, but only with dangerous & ugly code like ApiInvoker.set(customApiInvoker) but I refuse to provide such code ;))

[wing328]

Well I'm just sharing my view with you from a developer perceptive.

I'll let @fehguy to decide as this change would require developers who are already using the Java SDK to update their code.

[xhh]

@karussell regarding backward compatibility, what about providing a default ApiInvoker?

public class ApiInvoker {
    public static final ApiInvoker DEFAULT = new ApiInvoker();
}

public class PetApi {
    public PetApi() {
        this(ApiInvoker.DEFAULT);
    }

    public PetApi(ApiInvoker apiInvoker) {
        this.apiInvoker = apiInvoker;
    }
}

[karussell]

Good point, and better approach, still I wouldn't design my code this way as every static variable could lead to problems. E.g. here several instances could be used and some use the default and some the custom ... personally I would just break the compatibility e.g. when upgrading to 2.2

Another better approach in my eyes would be to have something like:

// this class contains the default apiInvoker which is not static 
// and a custom one can be provided via the constructor
MyApi api = new MyApi(/*new ApiInvoker(){...}*/); 
PetApi petApi = petapi.getPetAPI();
OtherApi otherApi = petapi.getOtherAPI();

[olensmar]

@xhh @karussell are we all set to merge here?

[xhh]

@olensmar my PR #791 includes the changes of making ApiInvoker (renamed to ApiClient) more pluggable as well as other improvements, such as changing static methods to instance methods so that those behaviors of the api client are customizable via overriding those instance methods.

So please consider merging that one.

[olensmar]

@xhh I would like to merge yours but also agree with @karussell that we should avoid static variables as much as possible - perhaps you could refactor in line with his suggestions?

[xhh]

@olensmar I can see your concerns on static variables, but if we do this

public class PetApi {
    public PetApi() {
        this(new ApiInvoker());
    }

    public PetApi(ApiInvoker apiInvoker) {
        this.apiInvoker = apiInvoker;
    }
}

then every time new PetApi(), new UserApi(), etc. would create a new instance (with the same functionalities and config), would not be necessary?
The idea here is to share a default api invoker, so that users don't have to store a shared one themselves and pass to every call to new PetApi(sharedApiInvoker).

Could you share more details on avoiding static variables here and generally? Then I'm happy to make the changes to avoid it.

[karussell]

I would either avoid the default constructor or better have a management class (SwaggerApi) which creates all the XYApi classes:

public class SwaggerApi {
    public SwaggerApi() {
        this(new ApiInvoker());
    }

    public SwaggerApi(ApiInvoker apiInvoker) {
        this.petApi = new PetApi(apiInvoker);      
    }

    public PetApi getPetApi() {
         return this.petApi;
    }
}

[olensmar]

@karussell @xhh that sounds good to me - we need to merge this today though so we can get it into the upcoming release - would either of you have time to do something in the line of this?

[wing328]

this one can be closed as @xhh has submitted #1046 to make Java API client more pluggable