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

Android volley library enhancement and tests #1920

Merged
merged 9 commits into from
Jan 22, 2016

Conversation

Shyri
Copy link
Contributor

@Shyri Shyri commented Jan 18, 2016

Hi! I have added some enhancements to android-volley-library generator. I want to add tests requested in #1839 but I have some questions so I am gonna make a to do list:

  • Add Synchronous Requests support
  • Improve RequestQueue configuration capabilities
  • Write units test for PetApi.

I will explain one by one:

Add Synchronous Requests support

The most common usage of Volley is making asynchronous requests with callbacks as there were already in the volley generator, something like:

api.getPetById(pet.getId(), 
    new Response.Listener<Pet>() {
            @Override
            public void onResponse(Pet response) {
                fetched[0] = response;
                waiter.resume();
            }
        }, new Response.ErrorListener() {
            @Override
            public void onErrorResponse(VolleyError error) {

            }
        });

But there are cases, like in services for instance, where you want to call that request synchronously. I've added this functionality so one can call both the way above or like android-default was:

Pet fetched = api.getPetById(pet.getId());

I've set a 30 seconds timeout, I don't know if this is ok with you. Should I make it configurable?

Improve RequestQueue configuration capabilities

With current version is necessary to provide a Context to ApiInvoker initializer so it can build a RequestQueue based on that Context. I've added to the initializer some parameters to configure the Volley request queue like:
ApiInvoker(Cache cache, Network network, int threadPoolSize, ResponseDelivery delivery). These are passed to RequestQueue constructor. Default would do something equivalent to:

``` java`
HttpStack stack = new HurlStack();
network = new BasicNetwork(stack);
cache = new NoCache();
mRequestQueue = new RequestQueue(cache, network)


that will use a default network thread pool size of 4, and a 

``` java
new ExecutorDelivery(new Handler(Looper.getMainLooper()))

as ResponseDelivery

Unit Tests

I've been testing the petstore.json.orig file. There are some issues: As OAuth2 support is not yet implemented I cannot get it working straight out of the generated code, as it expects to receive some value for auth keys. Is this the correct the correct behaviour? Should the client throw an exception when no authentication was provided but it is specified, like in deletePet

...
"security": [
          {
            "petstore_auth": [
              "write:pets",
              "read:pets"
            ]
          }
        ]
...

or should it let the request be made and let the server return failure?

Anyway, I've written the PetApiTest both for sync and async apis from a modified json removing oauth and adding apikey authentication in all methods. Once I have this clear I'll make the necessary changes and write the rest of the tests

@wing328
Copy link
Contributor

wing328 commented Jan 19, 2016

About timeout, yes please make it configurable. C#, Ruby, PHP, Java, etc allows developers to set the timeout.

@wing328
Copy link
Contributor

wing328 commented Jan 19, 2016

RE: should it let the request be made and let the server return failure?

General speaking, if the authentication setting (e.g. username, password, token, API key,etc) is not passed to the method via Configuration or other ways (e.g. set default header), then the API client (in Ruby, PHP, Java, etc) will still make the request and the server will send back an error (e.g. 401), which will be handled by the API client and throw an Api Exception accordingly.

@wing328
Copy link
Contributor

wing328 commented Jan 19, 2016

To add OAuth support to the Android client, you may want to have a look at at the Java Retrofit client: https://github.com/swagger-api/swagger-codegen/tree/master/samples/client/petstore/java/retrofit/src/main/java/io/swagger/client/auth

@wing328
Copy link
Contributor

wing328 commented Jan 19, 2016

For petstore.json, other API clients are using https://github.com/swagger-api/swagger-codegen/blob/master/modules/swagger-codegen/src/test/resources/2_0/petstore.json for unit testing.

I'm not familiar with petstore.json.org so I'm not sure if it's correct.

@Shyri
Copy link
Contributor Author

Shyri commented Jan 20, 2016

I've added configurable timeout, some fix for apikey check in order to let the request to be sent when no apikey was set.
Also I've added the PetApiTest unit tests and an OAuth stub so the tests can pass. If you agree I'm going to add OAuth support to the checklist so we can have a full featured generator out of this PR

@wing328
Copy link
Contributor

wing328 commented Jan 21, 2016

Thanks @Shyri I suggest we merge these changes into master now so as to gather feedback from the developers while you're working on the remaining items.

@Shyri
Copy link
Contributor Author

Shyri commented Jan 21, 2016

@wing328 That's fine by me :)

wing328 added a commit that referenced this pull request Jan 22, 2016
Android volley library enhancement and tests
@wing328 wing328 merged commit 03b463b into swagger-api:master Jan 22, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants