Add access to json payload in JsonAccessToken

[jmarkovic]

The following pull request adds a simple accessor method to the JsonAccessToken that provides the raw JSON response to the library user.

There are quite a few token responses that contain extra data valuable or mandatory for the library user. Since there is no exposure to that data, a simple accessor method like the one provided in this pull request fixes that problem,

This also potentially closes the #20 issue.

[dmfs]

Thanks for your PR. I see a few issues with this change though.

1. Returning the original JSONObject allows the caller to modify the JsonAccessToken instance. At present you don't get access to the JSONObject, so from your perspective the JsonAccessToken is immutable. This change would make it mutable.

   I know, technically it can share the mutable JSONObject with its creator, making it non-immutable. However, at present this is only created by TokenResponseHandler which doesn't keep a reference to the JSONObject.

2. The new method is not declared in the OAuth2AccessToken interface. There is also an ImplictGrantAccessToken which could also benefit from the ability to return additional token parameters, even though it's not based on JSON.

3. This change makes JSONObject a part of the public API. I'd like to keep this an implementation detail which is not part of the API (i.e. no interface depends on it and no constructor invoked by the library user requires it). It may well be that we're going to replace the JSON implementation some day and I don't want to break the API when we do.

When I wrote #20 I was thinking of a simple method which returns a String or CharSequence value for a given key. Of course, in JSON it may be possible that a value is not a String, so we'd just return the .toString() value (unless the value is null of course). Nowadays I would prefer returning an Optional<String> from our jems library, like so:

    Optional<String> parameter(String key);

The Optional would be absent if the JSONObject contains no such key or a null value.

Note that would still allow you to get nested JSON objects, you just would get them as a String having to parse it again

[dmfs]

@jmarkovic are you going to work on these changes? I can do it myself otherwise.

[jmarkovic]

@dmfs thanks for the comment and reminding me to reply. Apologies, I was unavailable for the whole weekend.

I will try to work on this ASAP and might contact you for your opinion.

I agree with pretty much everything you said. The fact that I'm exposing the JSONObject became obvious to me the moment I made the push request and immediately I started thinking about what would be a better approach (like toString as you mentioned). The Optional might even be a better guard here if you do not want to use annotations to mark the return value as a possible Nullable.

As for the point about the interface - I see what you want to say and I do not argue against it. However, I'm not sure what would be the best common API method here. My reasoning was quite simple on this point: an end user wants a token response for a specific endpoint, and the user knows what type to expect. Thus, while not the best solution, the user can have a type check and cast the accessToken parameter to the JSON variant and extract the string representing the JSON response.

Far from the ideal or clean approach, but in my current case, it proved to be good enough.

I will have some time to invest in this PR during this week. But if you wish, you can always take over. Personally, I am fairly interested in how will this develop.

[dmfs]

@jmarkovic sorry, I didn't mean to urge you.

Considering the use case I think it's acceptable to return String or an Optional thereof. I expect most services to return just additional Strings anyway. Numbers, booleans, arrays and objects would have to go through a serialization/de-serialization cycle, but these cases are most likely very rare and this is unlikely to be in a performance critical code path (the network request is usually the bottleneck). I wouldn't mind the minimal performance penalty in this case.

In case of ImplicitGrantAccessToken we only deal with character data, so it's not a limitation in this case.

Converting the value back is just a matter of:

Optional<JSONObject> argument = new Mapped(JSONObject::new, token.parameter("object-argument"));
Optional<Integer> argument = new Mapped(Integer::valueOf, token.parameter("int-argument"));

btw, this is my idea of an implementation in JsonAccessToken:

@Override
public Optional<String> parameter(String key) 
{
    return new Mapped<>(Object::toString, new NullSafe<>(mTokenResponse.opt(key)));
}

[jmarkovic]

@dmfs no need to apologize and thanks for the advice.

I used your explanation as a base for the second commit. I tried to follow the same code style the rest of the project has but feel free to inform me if I did a mistake somewhere.

With the last commit, OAuth2AccessToken has an Optional<CharSequence> extraParameter(final String parameterName) method. The signature has throws ProtocolException as other methods do. However, instead of using Optional<String> as discussed before, I opted for Optional<CharSequence> to be more in line with some other types, primarily used in ImplicitGrantAccessToken.

In ImplicitGrantAccessToken, I used OptionalParameter<CharSequence> similar to other methods. Since the TextValueType expects a CharSequence, I changed the interface return type from Optional<String> to Optional<CharSequence>. I could've created a new StringValueType, but in my opinion, that might be an unnecessary overhead. Please let me know what you think here.

For JsonAccessToken, I simply use NullSafe optional with mTokenResponse.optString(parameterName, null). That should take care of everything.

I had to dig through the code for a few minutes to get myself acquainted with the org.dmfs.optional and AFAIK this approach might be what you wanted.

[jmarkovic]

To be quite honest, I am not entirely sure how the ImplicitGrantAccessToken should work so at this point your help would be much appreciated.

[dmfs]

Hmm, I'll have a look at that. Could be a bug somewhere else.

[dmfs]

@jmarkovic sorry, the URL was wrong. The ? needs to be a #, i.e. http://localhost#state=1&key=value. That should fix it.

[dmfs]

btw, you can remove the ProtocolException from this method. I can't see any reason why we would see any here. Extra parameters are not part of the protocol and if a parameter is missing we just return an absent.

[jmarkovic]

@dmfs you were right about the link, the ? had to be replaced with #. Thanks for that!

I took some extra time to see why my local builds are failing and now I can run tests locally. I have fixed the ImplicitGrantAccessTokenTest and removed the ProtocolExtension from the method signature. Apologies for the long wait.

Hopefully, this resolves all the issues we have here, but you are more than welcome to comment.

[dmfs]

I'll prepare and release version 0.11 later this evening.

[jmarkovic]

Thank you! I will check for the update before the end of this week.