Skip to content

data model changes - #16189

Draft
sahusanket wants to merge 7 commits into
developfrom
CDAP-21261_refresh_token_rotation
Draft

data model changes#16189
sahusanket wants to merge 7 commits into
developfrom
CDAP-21261_refresh_token_rotation

Conversation

@sahusanket

Copy link
Copy Markdown
Contributor

No description provided.

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Code Review

This pull request introduces support for different OAuth types (STANDARD, PKCE, and REFRESH_TOKEN_ROTATION) within the CDAP data pipeline. It updates OAuthProvider, OAuthStore, PutOAuthProviderRequest, and OAuthHandler to handle, store, and retrieve the new oauthType field, defaulting to STANDARD. The feedback suggests several improvements: handling potential null values in PutOAuthProviderRequest's getter to prevent NullPointerExceptions during GSON deserialization, and removing redundant @nullable annotations and null checks in OAuthProvider and OAuthStore since oauthType is guaranteed to be non-null.

Comment on lines +83 to +85
public OAuthProvider.OAuthType getOAuthType() {
return oauthType;
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

high

When PutOAuthProviderRequest is deserialized from JSON (e.g., via GSON in OAuthHandler), GSON bypasses the constructor if there is no default constructor, leaving the oauthType field as null if it is missing from the request payload. To prevent potential NullPointerExceptions in callers, the getter should return a default value of OAuthType.STANDARD if oauthType is null.

Suggested change
public OAuthProvider.OAuthType getOAuthType() {
return oauthType;
}
public OAuthProvider.OAuthType getOAuthType() {
return oauthType == null ? OAuthProvider.OAuthType.STANDARD : oauthType;
}

Comment on lines +35 to +36
@Nullable
private final OAuthType oauthType;

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

medium

The oauthType field is annotated with @Nullable, but both constructors and the builder guarantee that it is initialized to a non-null value (defaulting to OAuthType.STANDARD if null). To avoid confusion and redundant null checks, the @Nullable annotation should be removed.

Suggested change
@Nullable
private final OAuthType oauthType;
private final OAuthType oauthType;

Comment on lines +90 to +93
@Nullable
public OAuthType getOAuthType() {
return oauthType;
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

medium

Since oauthType is guaranteed to be non-null, the getOAuthType() getter should not be annotated with @Nullable.

  public OAuthType getOAuthType() {
    return oauthType;
  }

Comment on lines +342 to +345
fields.add(Fields.stringField(
OAUTH_TYPE_COL,
Optional.ofNullable(oauthProvider.getOAuthType())
.orElse(OAuthProvider.OAuthType.STANDARD).toString()));

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

medium

Since OAuthProvider.getOAuthType() is guaranteed to be non-null, the redundant null check using Optional.ofNullable can be removed to simplify the code.

    fields.add(Fields.stringField(
            OAUTH_TYPE_COL,
            oauthProvider.getOAuthType().toString()));

@sahusanket
sahusanket force-pushed the CDAP-21261_refresh_token_rotation branch from 0d2ee57 to 641e6b6 Compare July 27, 2026 06:29
@sahusanket
sahusanket force-pushed the CDAP-21261_refresh_token_rotation branch 3 times, most recently from 4c7b74b to cec6114 Compare July 30, 2026 11:21
@sahusanket
sahusanket force-pushed the CDAP-21261_refresh_token_rotation branch from cec6114 to b8283ff Compare July 31, 2026 19:47
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.

1 participant