NIFI-14154 Add support for oAuth to GetWorkdayReport processor #9631
Conversation
|
Reviewing |
There was a problem hiding this comment.
Hi @sfc-gh-mgemra and thanks for the PR!
The code changes LGTM, will build and run some tests against a Workday instance. In the meantime, a couple of requests:
- can you revert changes that are purely import ordering, whitespace, indentation, etc? I wish we had some checkstyle rules in place for this but that would require a lot of work across the whole codebase and it's better to keep the changes as limited as possible.
- for the two new properties you added, please remove
displayNameand only usename. I know this is not consistent with the rest of the code for this processor but we are trying to go away from thisdisplayNameapproach. We should probably mark it as deprecated in the API.
There was a problem hiding this comment.
Thanks for the contribution @sfc-gh-mgemra, I provided some initial feedback on property naming and dependencies.
In general, the formatting changes should be reverted so that the scope of this pull request is clear.
| .build(); | ||
|
|
||
| public static AllowableValue BASIC_AUTH_TYPE = new AllowableValue( | ||
| "basic_auth", |
There was a problem hiding this comment.
The value field should follow enum conventions, so all capitals with BASIC_AUTH would be best in this scenario.
| ); | ||
|
|
||
| public static AllowableValue OAUTH_TYPE = new AllowableValue( | ||
| "oauth", |
There was a problem hiding this comment.
| "oauth", | |
| "OAUTH", |
| .name("auth-type") | ||
| .displayName("Authorization Type") |
There was a problem hiding this comment.
For new properties, the displayName is not needed and the name can be a human-readable name.
| .name("auth-type") | |
| .displayName("Authorization Type") | |
| .name("Authorization Type") |
| .name("Access Token Provider") | ||
| .displayName("OAuth2 Access Token Provider") |
There was a problem hiding this comment.
The displayName should be removed as noted on other properties.
| .name("Access Token Provider") | |
| .displayName("OAuth2 Access Token Provider") | |
| .name("Access Token Provider") |
| <dependency> | ||
| <groupId>org.apache.nifi</groupId> | ||
| <artifactId>nifi-oauth2-provider-api</artifactId> | ||
| <scope>compile</scope> |
There was a problem hiding this comment.
This scope should be changed to provided and declared before other test dependencies for consistency.
There was a problem hiding this comment.
I played with the code changes against an existing instance of Workday. Things were working as expected to get data out. However, when I configured the controller service for OAuth and before it was enabled for the first time, I used the Verify feature and it returned an error:
Cannot invoke "String.equals(Object)" because "this.grantType" is null.
This is definitely not related to this code change and we can file a follow up JIRA and deal with it separately. It feels like something is missing in terms of initialization for the verify call to be working as expected when the controller service has not been used yet.
There was a problem hiding this comment.
Thanks for making the adjustments @sfc-gh-mgemra, the latest version looks good! I will defer to @pvillard31 for merging.
sfc-gh-mgemra
force-pushed
the
nifi-14154
branch
from
9a95be3 to
8d71d2d
Compare
Summary
NIFI-14154
Currently, the GetWorkdayReport processor supports only Basic Auth. Adding support for oAuth.
Tracking
Please complete the following tracking steps prior to pull request creation.
Issue Tracking
Pull Request Tracking
NIFI-00000NIFI-00000Pull Request Formatting
mainbranchVerification
Please indicate the verification steps performed prior to pull request creation.
Build
mvn clean install -P contrib-checkLicensing
LICENSEandNOTICEfilesDocumentation