-
Notifications
You must be signed in to change notification settings - Fork 92
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
Adding feature to include token generation for long process of spark. #618
Conversation
+ If this contains client credential grant type variables then use auth else normal request + Added authentication in config for http dispatcher types
Is this implementing Bearer authentification? (https://datatracker.ietf.org/doc/html/rfc6750) I think it would make sense to have |
...src/main/scala/za/co/absa/spline/harvester/dispatcher/httpdispatcher/rest/RestEndpoint.scala
Outdated
Show resolved
Hide resolved
...src/main/scala/za/co/absa/spline/harvester/dispatcher/httpdispatcher/rest/RestEndpoint.scala
Outdated
Show resolved
Hide resolved
...src/main/scala/za/co/absa/spline/harvester/dispatcher/httpdispatcher/rest/RestEndpoint.scala
Outdated
Show resolved
Hide resolved
Currently, the build is failing because test do not compile. The tests must be modified to accommodate the changes in the dispatcher. |
+ Added functionality to check if token expired and only recreate token if expired
…t in authentication map
Added AuthenticationFactory.scala to create auth class as per argumen…
@cerveada Tests are fixed and i have tested the jar, it works fine. can you please check |
@rupesh3020 thanks for fixing the tests and other issues, I will take a look. |
core/src/main/scala/za/co/absa/spline/harvester/dispatcher/httpdispatcher/rest/RestClient.scala
Outdated
Show resolved
Hide resolved
...scala/za/co/absa/spline/harvester/dispatcher/httpdispatcher/rest/AuthenticationFactory.scala
Outdated
Show resolved
Hide resolved
...scala/za/co/absa/spline/harvester/dispatcher/httpdispatcher/rest/AuthenticationFactory.scala
Outdated
Show resolved
Hide resolved
...scala/za/co/absa/spline/harvester/dispatcher/httpdispatcher/rest/AuthenticationFactory.scala
Outdated
Show resolved
Hide resolved
...scala/za/co/absa/spline/harvester/dispatcher/httpdispatcher/rest/AuthenticationFactory.scala
Outdated
Show resolved
Hide resolved
@cerveada Issues fixed. Can you please check once. I have done functional testing |
...scala/za/co/absa/spline/harvester/dispatcher/httpdispatcher/rest/AuthenticationFactory.scala
Outdated
Show resolved
Hide resolved
...scala/za/co/absa/spline/harvester/dispatcher/httpdispatcher/rest/AuthenticationFactory.scala
Outdated
Show resolved
Hide resolved
...scala/za/co/absa/spline/harvester/dispatcher/httpdispatcher/rest/AuthenticationFactory.scala
Outdated
Show resolved
Hide resolved
...scala/za/co/absa/spline/harvester/dispatcher/httpdispatcher/rest/AuthenticationFactory.scala
Outdated
Show resolved
Hide resolved
case Some(map: scala.collection.immutable.Map[String, Any]) => | ||
val token = map.getOrElse("access_token", "").toString | ||
if (token.nonEmpty) { | ||
val expirationTime = Instant.now().plus(Duration.ofSeconds(map.getOrElse("expires_in", 0).toString.toLong)) | ||
val newToken = Token(token, expirationTime) | ||
tokenCache = Some(newToken) | ||
} | ||
token |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In this case access_token
is mandatory in the response(https://www.rfc-editor.org/rfc/rfc6749#section-5.1), so you may just assume it is there.
No need for map.getOrElse("access_token", "")
or checking token.nonEmpty
.
Just use map("access_token")
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
throw new RuntimeException(failureMessage) | ||
} | ||
} else { | ||
tokenCache.getOrElse(throw new RuntimeException(failureMessage)).tokenValue |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
in this place isTokenInvalid
was already checked, so you can safely do tokenCache.get.tokenValue
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@cerveada done
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@cerveada Can you please merge if everything looks good as we are waiting for this feature.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Sorry guys, I was really buried with other stuff recently, but I really would like to have a quick look it it. Give me an hour or two, I'll check it out. In general, yes, that's awesome feature, I'm not going to delay merge. |
Kudos, SonarCloud Quality Gate passed! |
@rupesh3020 Thank you very much for your contribution!. We've made a few changes to your code and merged it as another pull request - #631. So I'm closing this one for that reason. |
As we have feature to add HTTP headers in the header configuration but this token expires in one hour and spline does not have a mechanism to recreate token and make requests.
This feature adds the functionality by adding a new configuration authentication, where we can specify