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

Reintroducing OslcOauthClient from oslc-java-client to oslc4j-client library #77

Merged
merged 4 commits into from Nov 13, 2019

Conversation

@jadelkhoury
Copy link
Contributor

jadelkhoury commented Sep 30, 2019

Here are the major changes in this reintroduction:

lyo.client\oslc4j-client\src...\oslc4j\client\RootServicesHelper.java

  • Copied from lyo.client\oslc-java-client\src...\client\oslc\jazz\JazzRootServicesHelper.java
  • Renamed, since it is not related to JAZZ.
  • Did not include the initFormClient() method
  • Added some Consumer-related helper methods.

C:\Users\jelx25\git\lyo\lyo.client\oslc4j-client\src...\oslc4j\client\OslcClient.java

  • Removed all code relating to "getCatalogUrl". This is in the rootServiceHelper class.
  • getResource() & lookupServiceProviderUrl() methods need to throw exceptions: IOException, OAuthException, URISyntaxException

lyo.client\oslc4j-client\src...\oslc4j\client\OslcOAuthClient.java

  • Copied from lyo.client\oslc-java-client\src...\client\oslc\OslcOAuthClient.java
  • Introduced all getResource() methods to override all getResource() methods from parent. Otherwise, the authorisation header is not included when super method is called directly.
    • Is this an old bug?
  • Did not implement update/create methods.
  • getResourceInternal() creates a new httpClient, instead of using a clientPool. Is that a problem?
@jadelkhoury jadelkhoury requested review from jamsden and berezovskyi Sep 30, 2019
@jadelkhoury jadelkhoury self-assigned this Sep 30, 2019
@berezovskyi

This comment has been minimized.

Copy link
Member

berezovskyi commented Sep 30, 2019

I think we need to introduce an interface to make two clients interchangeable where possible.

Copy link
Contributor

jamsden left a comment

Jad,
I don't think this is the right approach. RootServicesHelper.java is obsolete, its functionality was intentionally moved into OslcClient as root services is fundamental to discovering discovery in the jazz.net apps. We have also discussed the possibility of adding a root services vocabulary to OSLC Core for this purpose.

Authenticators implements ClientRequestFilter, ClientResponseFilter to capture and handle authentication challenges. It may not be possible to do this with OAuth challenges, but we should try.

@jadelkhoury

This comment has been minimized.

Copy link
Contributor Author

jadelkhoury commented Oct 15, 2019

Hi Jim! I am not sure how moving RootServicesHelper.java functionality into OslcClient helps. I needed this functionality even when I don't have an OslcClient. If we want to promote rootServices, why not make it more visible in the SDK with its own class, instead of hiding it within OslcClient?

@berezovskyi ! I can revert (almost) all changes to OslcClient, if I no longer make OslcOAthClient a subclass of OslcClient(). Instead OslcOAthClient has an attribute of type OslcClient that is then used to do the REST calls, once authenticated.
But the central problem remains: the methods getResource() cannot share a common signature, since OslcOAuthClient needs to throw oauth exceptions. that is, I cannot define a common itnerface for both OslcOAthClient and OslcClient.

@jadelkhoury

This comment has been minimized.

Copy link
Contributor Author

jadelkhoury commented Oct 15, 2019

@berezovskyi
I read now on the mailing list your point about defining a LyoResponse return value.
We might need to do this together (to get it right the first time :-)) But I can prepare the grounds by reverting some of the changes on this branch.

Both OslcOAuthClient and OslcClient share a common interface.
@jadelkhoury

This comment has been minimized.

Copy link
Contributor Author

jadelkhoury commented Oct 28, 2019

This is not 100% complete, but I wonder if this new structure is more acceptable.

Please explore the proposed client application on OSLC/lyo-samples#10.

@berezovskyi

This comment has been minimized.

Copy link
Member

berezovskyi commented Nov 5, 2019

TODO:

  • throw IllegalStateException if the authorisation has not been performed. as it is, you throw IllegalArguemntException
  • fix OslcClient merge conflict
@jadelkhoury

This comment has been minimized.

Copy link
Contributor Author

jadelkhoury commented Nov 11, 2019

TODO:
* [ ] throw IllegalStateException if the authorisation has not been performed. as it is, you throw IllegalArguemntException
* [ ] fix OslcClient merge conflict

Fixed. But @berezovskyi ! Sure we should not fix some class names before merging?

@jadelkhoury jadelkhoury merged commit a1477bc into master Nov 13, 2019
3 checks passed
3 checks passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
eclipsefdn/eca The author(s) of the pull request is covered by necessary legal agreements in order to proceed!
Details
@jadelkhoury jadelkhoury deleted the oauth branch Nov 13, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants
You can’t perform that action at this time.