[keycloak-dev] Remember to close responses using admin client

[Hynek Mlnarik]

On 10/21/2016 02:51 PM, Stan Silvert wrote:
> On 10/21/2016 6:09 AM, Marek Posolda wrote:
>> On 21/10/16 08:18, Stian Thorgersen wrote:
>>> For methods on the admin client that return a Response it's important to
>>> remember to close it. Failing to do this causes:
>>>
>>> * Not freeing up connections
>>> * Tests can fail intermittently as the tx may not be completed before you
>>> move on
>>>
>>> Ideal would be to find a way to prevent this and have the admin client
>>> close the responses, but I don't think that's possible.
>> Maybe we already discuss,  that we can possibly avoid using "Response"
>> object at most admin client operations.
>>
>> We typically use them in "create" methods (POST requests). So if we
>> refactor server-side to return the actual created object with filled ID,
>> we can use something like:
>>
>> ClientRepresentation createdClient =
>> realmResource.clients().create(clientRepresentation);
>> String createdUuid = createdClient.getId();
>>
>> instead of current:
>>
>> Response response = realm.clients().create(rep);
>> response.close();
>> String createdUuid = ApiUtil.getCreatedId(response);
>>
>>
>> The advantages is that:
>> - No need to care about closing response
>> - Better exception handling - instead of checking response status etc,
>> the new code will throw exception directly. Which is usually cleaner
>> - You don't need to send separate GET request to retrieve newly created
>> client. And in most cases, you usually want newly created client
>>
>> The possible disadvantage:
>> - The POST response is bigger as it contains representation of newly
>> created client. However you probably usually need the new client anyway.
>> Not sure if it's possible to set admin console (angular) to avoid
>> sending separate GET request when the entity was already retrieved
>> through POST, but I suppose that yes.
>>
>> Marek
> Anything is possible, but that's not what is expected from a POST
> request.   From RFC-7231:
>
> If one or more resources has been created on the origin server as a
>      result of successfully processing a POST request, the origin server
>      SHOULD send a 201 (Created) response containing a Location header
>      field that provides an identifier for the primary resource created
>      (Section 7.1.2  <https://tools.ietf.org/html/rfc7231#section-7.1.2>) and a representation that describes the status of the
>      request while referring to the new resource(s).
>
> So basically, the expected result is the identifier of the created
> resource and not the resource itself.

I second Marek's suggestions, it would also make API cleaner. AFAIR the 
RFC quote refers to Location header field, not the response body. So 
returning the ID in the Location header field (as it is now already) 
plus the detail of created object in the body would be in line with the 
spec. There are other samples of APIs returning object detail in the 
body when returning 201 Created, e.g. [1, 2]

--Hynek

[1] https://wiki.openstack.org/wiki/Neutron/APIv2-specification
[2] 
https://access.redhat.com/documentation/en-US/Red_Hat_Enterprise_Virtualization/3.6/html-single/REST_API_Guide/

>
> Maybe the problem can be solved by letting the admin client cache the
> responses before returning them to the caller.  Then our test harness
> can clean them up.
>>> _______________________________________________
>>> keycloak-dev mailing list
>>> keycloak-dev at lists.jboss.org
>>> https://lists.jboss.org/mailman/listinfo/keycloak-dev
>> _______________________________________________
>> keycloak-dev mailing list
>> keycloak-dev at lists.jboss.org
>> https://lists.jboss.org/mailman/listinfo/keycloak-dev
> _______________________________________________
> keycloak-dev mailing list
> keycloak-dev at lists.jboss.org
> https://lists.jboss.org/mailman/listinfo/keycloak-dev