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

[JAVA][Feign] Replace Apache oltu with scribejava #8318

Conversation

hugo-ma-alves
Copy link
Contributor

@hugo-ma-alves hugo-ma-alves commented Jan 4, 2021

Hello

This PR is the implementation for the issue #8297.

To sum up this PR addresses the following issues:

  • Replaces apache OLTU with scribejava
  • Implements Oauth client_credentials grant and password flow.
    I can also implement the implicit flow. But I am not sure how to handle the user redirections in this context. These changes focus more on server to server communication. I Believe that the implicit flow is considered deprecated, it should be replaced by the authorization code grant.

The following git repo contains the Petstore client generated using the code on this PR. The open api spec file is also on the git repo
https://github.com/hugo-ma-alves/openapi-generator-petstore-demo

According with the ticket #6158 the support for java 7 is not guaranteed anymore right? I did some java 8 only code, so it won't work on jre7. But it should be easy to revert to java 7.

@bbdouglas (2017/07) @sreeshas (2017/08) @jfiala (2017/08) @lukoyanov (2017/09) @cbornet (2017/09) @jeff9finger (2018/01) @karismann (2019/03) @Zomzog (2019/04) @lwlee2608 (2019/10) @bkabrda (2020/01)

btw, I committed the changes on the samples folder, not sure if they should also be on git. Let me know if I should remove them.

PR checklist

  • Read the contribution guidelines.
  • Pull Request title clearly describes the work in the pull request and Pull Request description provides details about how to validate the work. Missing information here may result in delayed response from the community.
  • If contributing template-only or documentation-only changes which will change sample output, build the project beforehand.
  • Run the shell script ./bin/generate-samples.shto update all Petstore samples related to your fix. This is important, as CI jobs will verify all generator outputs of your HEAD commit as it would merge with master. These must match the expectations made by your contribution. You may regenerate an individual generator by passing the relevant config(s) as an argument to the script, for example ./bin/generate-samples.sh bin/configs/java*. For Windows users, please run the script in Git BASH.
  • File the PR against the correct branch: master, 5.1.x, 6.0.x
  • Copy the technical committee to review the pull request if your PR is targeting a particular programming language.

- Implement the following authentication methods
  - ApiKey header
  - HTTP basic authentication
  - Oauth client credentials flow
  - Oauth Implicit flow
  - Oauth Pasword (deprecated)
@hugo-ma-alves hugo-ma-alves changed the title [JAVA][Feign}Replace Apache oltu with scribejava [JAVA][Feign] Replace Apache oltu with scribejava Jan 4, 2021
@wing328
Copy link
Member

wing328 commented Jan 5, 2021

According with the ticket #6158 the support for java 7 is not guaranteed anymore right? I did some java 8 only code, so it won't work on jre7. But it should be easy to revert to java 7.

That's correct

@wing328
Copy link
Member

wing328 commented Jan 7, 2021

CircleCI reports the following errors:

[ERROR] Failed to execute goal org.apache.maven.plugins:maven-javadoc-plugin:3.1.1:jar (attach-javadocs) on project petstore-feign: MavenReportException: Error while generating Javadoc:
[ERROR] Exit code: 1 - /home/circleci/OpenAPITools/openapi-generator/samples/client/petstore/java/feign/src/main/java/org/openapitools/client/ApiClient.java:294: error: lambda expressions are not supported in -source 1.7
[ERROR] .filter(requestInterceptor -> type.isAssignableFrom(requestInterceptor.getClass()))
[ERROR] ^
[ERROR] (use -source 8 or higher to enable lambda expressions)
[ERROR] 
[ERROR] Command line was: /usr/lib/jvm/java-8-openjdk-amd64/jre/../bin/javadoc @options @packages
[ERROR] 
[ERROR] Refer to the generated Javadoc files in '/home/circleci/OpenAPITools/openapi-generator/samples/client/petstore/java/feign/target/apidocs' dir.
[ERROR] -> [Help 1]
[ERROR] 
[ERROR] To see the full stack trace of the errors, re-run Maven with the -e switch.
[ERROR] Re-run Maven using the -X switch to enable full debug logging.
[ERROR] 
[ERROR] For more information about the errors and possible solutions, please read the following articles:
[ERROR] [Help 1] http://cwiki.apache.org/confluence/display/MAVEN/MojoExecutionException
[ERROR] 
[ERROR] After correcting the problems, you can resume the build with the command
[ERROR]   mvn <goals> -rf :petstore-feign

Can you please take a look when you've time?

@hugo-ma-alves
Copy link
Contributor Author

Forgot to remove the support for java 1.7 from the pom 😞 . Because I introduced lambda expressions it won't work anymore for java<1.8
I pushed some changes, let see if it works.

@hugo-ma-alves
Copy link
Contributor Author

Now it is throwing some timeout error.

21:36:20.954 [qtp2074407503-12] INFO  i.s.sample.resource.PetStoreResource - placeOrder ID 1001 STATUS pending
21:37:38.877 [qtp2074407503-10] INFO  i.s.sample.resource.UserResource - createUser ID 11222 STATUS scala-test-username
21:37:39.143 [qtp2074407503-11] INFO  i.s.sample.resource.UserResource - createUsersWithArrayInput
21:37:39.181 [qtp2074407503-14] INFO  i.s.sample.resource.UserResource - createUsersWithListInput
21:37:39.226 [qtp2074407503-15] INFO  i.s.sample.resource.UserResource - createUser ID 4000 STATUS tony
21:37:39.241 [qtp2074407503-14] INFO  i.s.sample.resource.UserResource - updateUser ID 4000 STATUS tony

Too long with no output (exceeded 10m0s): context deadline exceeded```

It looks related with the scala module? Not sure what the problem is in this case.

@wing328
Copy link
Member

wing328 commented Jan 10, 2021

No worry. We can safely remove java6, java7 support in the Feign client

Please update the samples when you've time to fix the following:

--- a/samples/client/petstore/java/feign-no-nullable/README.md
+++ b/samples/client/petstore/java/feign-no-nullable/README.md
@@ -75,4 +75,3 @@ It's recommended to create an instance of `ApiClient` per thread in a multithrea
 
 
 
-
diff --git a/samples/client/petstore/java/feign/README.md b/samples/client/petstore/java/feign/README.md
index afd609fe63..fa5dd565da 100644
--- a/samples/client/petstore/java/feign/README.md
+++ b/samples/client/petstore/java/feign/README.md
@@ -75,4 +75,3 @@ It's recommended to create an instance of `ApiClient` per thread in a multithrea
 
 
 
-
Perform git status
On branch feature/replace-apache-oltu-with-scribejava
Changes not staged for commit:
  (use "git add <file>..." to update what will be committed)
  (use "git checkout -- <file>..." to discard changes in working directory)

	modified:   samples/client/petstore/java/feign-no-nullable/README.md
	modified:   samples/client/petstore/java/feign/README.md

@hugo-ma-alves
Copy link
Contributor Author

Done, meanwhile I also updated the mustache template for gradle and sbt. They still had references for apache oltu.

@wing328
Copy link
Member

wing328 commented Jan 18, 2021

--- a/samples/client/petstore/java/feign-no-nullable/build.gradle
+++ b/samples/client/petstore/java/feign-no-nullable/build.gradle
@@ -116,6 +116,7 @@ dependencies {
     implementation "com.fasterxml.jackson.core:jackson-annotations:$jackson_version"
     implementation "com.fasterxml.jackson.core:jackson-databind:$jackson_databind_version"
     implementation "com.github.joschi.jackson:jackson-datatype-threetenbp:$jackson_threetenbp_version"
+    implementation "com.brsanthu:migbase64:2.2"
     implementation "com.github.scribejava:scribejava-core:$scribejava_version"
     implementation "com.brsanthu:migbase64:2.2"
     implementation 'javax.annotation:javax.annotation-api:1.3.2'
diff --git a/samples/client/petstore/java/feign/build.gradle b/samples/client/petstore/java/feign/build.gradle
index e077363990..637b0f9d64 100644
--- a/samples/client/petstore/java/feign/build.gradle
+++ b/samples/client/petstore/java/feign/build.gradle
@@ -118,6 +118,7 @@ dependencies {
     implementation "com.fasterxml.jackson.core:jackson-databind:$jackson_databind_version"
     implementation "org.openapitools:jackson-databind-nullable:$jackson_databind_nullable_version"
     implementation "com.github.joschi.jackson:jackson-datatype-threetenbp:$jackson_threetenbp_version"
+    implementation "com.brsanthu:migbase64:2.2"
     implementation "com.github.scribejava:scribejava-core:$scribejava_version"
     implementation "com.brsanthu:migbase64:2.2"
     implementation 'javax.annotation:javax.annotation-api:1.3.2'
Perform git status
On branch pull/8318
Changes not staged for commit:
  (use "git add <file>..." to update what will be committed)
  (use "git checkout -- <file>..." to discard changes in working directory)

	modified:   samples/client/petstore/java/feign-no-nullable/build.gradle
	modified:   samples/client/petstore/java/feign/build.gradle

no changes added to commit (use "git add" and/or "git commit -a")

As reported by the CircleCI, the samples are not yet updated. Let me know if you need help on that.

@hugo-ma-alves
Copy link
Contributor Author

I merged the master branch into this branch and forgot to update the samples.
All done now.

@wing328 wing328 merged commit ede2a23 into OpenAPITools:master Jan 19, 2021

:mainEnd
if "%OS%"=="Windows_NT" endlocal

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like this is not the latest gradlew.bat. I'll file another PR to update it.

cc @jimschubert

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Filed #8469

@pPanda-beta
Copy link

@hugo-ma-alves
Copy link
Contributor Author

@pPanda-beta
Yep, that is fixed by #9472

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.

None yet

3 participants