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

changed scope of dependencies in oauth-webapp to avoid inclusion multiple times during runtime #439

Merged
merged 1 commit into from
Dec 8, 2023

Conversation

Jad-el-khoury
Copy link
Contributor

changed scope of dependencies in oauth-webapp to avoid inclusion multiple times during runtime

Checklist

  • This PR adds an entry to the CHANGELOG. See https://keepachangelog.com/en/1.0.0/ for instructions. Minor edits are exempt.
  • This PR was tested on at least one Lyo OSLC server (e.g. manual workflow on Lyo Sample and OSLC RefImpl) or adds unit/integration tests.
  • This PR does NOT break the API

@Jad-el-khoury
Copy link
Contributor Author

This change will remove the many many warnings we get when starting a server with oauth support.

Example warnings:

[WARNING] org.apache.jena.sparql.lang.sparql_10.Token scanned from multiple locations: jar:file:///C:/Users/JadEl-khoury/.m2/repository/org/apache/jena/jena-arq/4.8.0/jena-arq-4.8.0.jar!/org/apache/jena/sparql/lang/sparql_10/Token.class, jar:file:///C:/Users/JadEl-khoury/git/lyo/refimpl/src/server-cm/target/jetty_overlays/oauth-webapp-6_0_0-SNAPSHOT_war/WEB-INF/lib/jena-arq-4.8.0.jar!/org/apache/jena/sparql/lang/sparql_10/Token.class

[WARNING] org.apache.jena.sparql.lang.sparql_10.TokenMgrError scanned from multiple locations: jar:file:///C:/Users/JadEl-khoury/.m2/repository/org/apache/jena/jena-arq/4.8.0/jena-arq-4.8.0.jar!/org/apache/jena/sparql/lang/sparql_10/TokenMgrError.class, jar:file:///C:/Users/JadEl-khoury/git/lyo/refimpl/src/server-cm/target/jetty_overlays/oauth-webapp-6_0_0-SNAPSHOT_war/WEB-INF/lib/jena-arq-4.8.0.jar!/org/apache/jena/sparql/lang/sparql_10/TokenMgrError.class

[WARNING] org.apache.jena.sparql.lang.sparql_11.JavaCharStream scanned from multiple locations: jar:file:///C:/Users/JadEl-khoury/.m2/repository/org/apache/jena/jena-arq/4.8.0/jena-arq-4.8.0.jar!/org/apache/jena/sparql/lang/sparql_11/JavaCharStream.class, jar:file:///C:/Users/JadEl-khoury/git/lyo/refimpl/src/server-cm/target/jetty_overlays/oauth-webapp-6_0_0-SNAPSHOT_war/WEB-INF/lib/jena-arq-4.8.0.jar!/org/apache/jena/sparql/lang/sparql_11/JavaCharStream.class

@berezovskyi
Copy link
Contributor

Jad, I like the PR but feels like the note is insufficient. It's a potentially breaking change for some, risking to bring one of the worst exceptions: class not found exception.

I am assuming those who include the oauth-core and/or use Designer will not be affected? Who will be?

Best if we change the usage guide to include this module and its assumed dependencies, leaving users to skip the deps they already have.

@jadelkhoury
Copy link
Contributor

Best if we change the usage guide to include this module and its assumed dependencies, leaving users to skip the deps they already have.

I am not sure what you mean by this suggestion.
If a user of oauth-webapp does not also include the dependency on - for example - oauth-core, won't they be using an indirect dependency on oauth-core? With this fix, we are just making sure they do state the depedencies they are using.

@berezovskyi
Copy link
Contributor

With this fix, we are just making sure they do state the depedencies they are using.

But this change says that the oauth-webapp can ignore missing classes, assuring that the user will provide them in the final POM. That's not what you expect in Maven by default. When you include a dependency on guava, for example, you expect it to bring all of its dependencies instead of throwing java.lang.ClassNotFoundException all over the place.

That's why I think we need to add a clear warning what a potential user of oauth-webapp needs to do not to get those exceptions.

@Jad-el-khoury
Copy link
Contributor Author

That's why I think we need to add a clear warning what a potential user of oauth-webapp needs to do not to get those exceptions.

And where/how do you suggest we have this warning?

@berezovskyi
Copy link
Contributor

berezovskyi commented Dec 7, 2023

@Jad-el-khoury
Copy link
Contributor Author

Option 2 https://oslc.github.io/developing-oslc-applications/eclipse_lyo/setup-an-oslc-provider-consumer-application#oslc-oauth-support

@berezovskyi ! This option already does that. It states that one needs to include all 3 dependencies (and that's LyoDesigner already does).

@Jad-el-khoury
Copy link
Contributor Author

Thanks you sir! I always feel proud (of myself) when I get an PR approved through your reviews.
Without even any changes -> 3 extra points.

@Jad-el-khoury Jad-el-khoury merged commit 6219a59 into master Dec 8, 2023
5 checks passed
@Jad-el-khoury Jad-el-khoury deleted the oauth-webapp branch December 8, 2023 11:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants