JDBC sample app to show use of Auth Tokens to connect the database and get the tokens from endpoints #23

rvpanchal1202 · 2024-05-10T07:11:02Z

-> Review Comments are addressed
-> Testing of functionality is done with Keyclock IDP

… token using the refresh token, we can get the new access token in case of a wrong access token or an expired access token. Added API to get the new access_token using refresh_token note: When we request for new access_token using refresh token, This request will give us both the tokens. which we can save later to original tokens and start using new Tokens instead of old tokens

Changes in the client application done as per comments and discussion. SetUpDbForOAuth() -> Adding authentication record in Vertica database is having issues. It's dropping the user and authentication method, but while setting the Authentication record, there is some error because of which Token authentication is failing and Database connection with access token is unable to establish. Without SetUpDbForOAuth() method call, Application is woking good.

Commented SetUpDbForOAuth() and TearDown() call.

@commandline

-> Removed all @commandline and picoli imports -> Removed all the blank lines from the file -> Indentation issue resolved. Note:Please adjust the tab stop as 4 to view the file content properly indented. -> Removed if else for first element in the map for framing the connectionString. -> All the Start braces and end braces are aligned and made the same in every place -> Added null check for an access token and refresh token after getting it from token URL

Added config.properties file which would be required for running the sample app. User have to update the values as per configuration. Note: Given Sample example for the Connection string. Because Connection string will be accepted in this format. <KeyName>=<Value>;<KeyName>=<Value>;......

-> Added functions for the redundant code for copying data to the buffered output stream. -> removed unnecessary imports.

Removed ClientID, ClientSecret, Tokenurl from getconnection Call.

Added Line between imports and class

Debug prints removed

Deprecated method use JsonParser().parse(res); removed. used JsonParser.parseString(res);

- Added Fix for setup of Authentication record to Database. - Uncommented DBSetup and Teardown for Authentication setup before OAuth sample test run

Added DiscoveryUrl to be provided in config.properties file for OAuth Authentication

Added RefreshToken check and If both the tokens are available, then it will use those tokens, Or else it will get new tokens using password grant.

Check if access token is not available, Get the tokens. else if a refresh token is not available, Get the tokens. else set both the tokens to properties.

Indentation issue resolved

Indentation resolved

Comment indentation resolved

Internal review comments are addressed. -> Redundant code for getting parameters from connection string addressed. -> Formating of comments for method explanation addressed.

Review comments are addressed. Changes ensureAccessToken as per suggestion given Comments and print statements are added

-> Spacing between Functions and indentation formating is done

JDBC/OAuthSampleApp/src/main/java/OAuthSampleApp.java

- Unused variable CONNECTION_STRING removed. - Added comments for the required calls - Added Refresh token check code in EnsureAccessToken Call

JDBC/OAuthSampleApp/src/main/java/OAuthSampleApp.java

Refresh Token check is removed as it might be that we will get only access Token and not refresh token. As per comment from Kevin.

-> Updated comment for EnsureAccessToken

Changed the function name from connectToDb to getConnection.

JDBC/OAuthSampleApp/src/main/java/OAuthSampleApp.java

Added null and empty checks for refresh token to save in the properties.

kevinkarch88 · 2024-05-22T12:59:38Z

I don't have any major issues with the code as is, it should work as intended.

DMickens · 2024-05-23T14:40:19Z

JDBC/OAuthSampleApp/src/main/java/OAuthSampleApp.java

+public class OAuthSampleApp {
+	private static Properties prop;
+	private static Map<String, String> csProp = new HashMap<String, String>();
+	private static String OAUTH_ACCESS_TOKEN_VAR_STRING = "OAUTH_SAMPLE_ACCESS_TOKEN";


In java we declare constants as final so the constant properties here should be private static final

Constants are also generally grouped together and declared first in a class, so I would move these two constants to be the first properties defined in the class followed by a newline and then followed by the rest of the properties

Changed OAUTH_ACCESS_TOKEN_VAR_STRING, OAUTH_REFRESH_TOKEN_VAR_STRING to constant "private static final"

DMickens · 2024-05-23T14:44:41Z

JDBC/OAuthSampleApp/src/main/java/OAuthSampleApp.java

+import com.google.gson.JsonObject;
+import com.vertica.jdbc.VerticaConnection;
+
+public class OAuthSampleApp {


Unless it is just rendering poorly in github, the spacing in this class is all off. Indentation should simply be four spaces per indentation level and they should be actual spaces, not tabs.

Hi Danny,
This indentation formatting is done using the auto-indent feature(Source->Format) in Eclipse. So If we make any changes here in git for indentation, It will reflect wrong when we open the file in Eclipse or other IDE.

If you are going to use the auto indent feature you need to update it to use four spaces instead of tabs, there should be an option for this. A space is a space, it's the same everywhere. A tab character may not render the same everywhere like we see here

Ok. As per the last commit, All indentation was good and no issues observed. However, I have changed the indentation from the tabs to Spaces and committed the changes. :)

I'm still seeing tabs in the PR. There is a setting in eclipse to use spaces instead of tabs when formatting code. If you have not enabled that, I suggest you change that and format the code again.

DMickens · 2024-05-23T14:52:33Z

JDBC/OAuthSampleApp/src/main/java/OAuthSampleApp.java

+
+	// Add/Create Authentication record in database. Create User and grant the
+	// permissions for User
+	private static void SetUpDbForOAuth() throws Exception {


In java convention for method names is to use camelCase. It's okay for the casing of the names in this sample to differ from the names in the ADO implementation in order to follow the convention. Please ensure all method names start with a lowercase letter and adhere to camelCase.

Changed all method names to camelCase.

DMickens · 2024-05-23T14:54:20Z

JDBC/OAuthSampleApp/src/main/java/OAuthSampleApp.java

+	private static void SetUpDbForOAuth() throws Exception {
+		String connectionString = getConnectionString();
+		vConnection = DriverManager.getConnection(connectionString);
+		String USER = prop.getProperty("User");


As these are not constant it should not be in all caps with underscore spacing. Please change these to be camelCase

Changed non constant vars to camelCase.

DMickens · 2024-05-23T14:57:25Z

JDBC/OAuthSampleApp/src/main/java/OAuthSampleApp.java

+		st.execute("DROP USER IF EXISTS " + USER + " CASCADE;");
+		st.execute("DROP AUTHENTICATION IF EXISTS v_oauth CASCADE;");
+		st.execute("CREATE AUTHENTICATION v_oauth METHOD 'oauth' LOCAL;");
+		st.execute("ALTER AUTHENTICATION v_oauth SET client_id= '" + CLIENT_ID + "';");


nit: either none should have spaces before the equal sign, or they all should. I would prefer that they all do, but lets make them consistent

DMickens · 2024-05-23T14:59:58Z

JDBC/OAuthSampleApp/src/main/java/OAuthSampleApp.java

+		String CLIENT_SECRET = prop.getProperty("ClientSecret");
+		String DISCOVERY_URL = prop.getProperty("DiscoveryUrl");
+		Statement st = vConnection.createStatement();
+		st.execute("DROP USER IF EXISTS " + USER + " CASCADE;");


Can you put all of these commands into an array of strings, then iterate through the array and call st.execute() for each element of the array. I asked Kevin to make that change in the ADO sample previously as well. It makes it easier to see everything that is being executed and it's easy to modify if need be.

Added queries to a string array and executed the queries using iterating through the array.

DMickens · 2024-05-23T15:00:31Z

JDBC/OAuthSampleApp/src/main/java/OAuthSampleApp.java

+	private static void TearDown() {
+		try {
+			Statement st = vConnection.createStatement();
+			String USER = prop.getProperty("User");


not a constant, change to user

DMickens · 2024-05-23T15:06:10Z

JDBC/OAuthSampleApp/src/main/java/OAuthSampleApp.java

+	// If access token is Invalid/Expired, Get new tokens using password/refresh grant
+	private static void EnsureAccessToken() throws Exception {
+		try {
+			String accessToken = System.getenv(OAUTH_ACCESS_TOKEN_VAR_STRING);


System.getenv will not throw an exception, just return null. So these should be moved outside of the try block

Moved System.getenv outside of try block

DMickens · 2024-05-23T15:10:26Z

I really want to be able to compare this side by side with the ado.net sample application, but the spacing issue is making that very difficult. When splitting the screen to review side by side, this code is stretched out and split over many lines with large spaces in between sections of code and it makes it difficult to review. Would you mind fixing that issue first and committing the change? Then I will finish my review, I'm not sure if it's an issue from using tabs or just too many spaces or what but making indentation level 4 manual spaces should fix it.

rvpanchal1202 · 2024-05-28T07:37:50Z

Code rearrangement of methods are done with respect to ADO.Net Sample App.

Review comments addressed, -> Code alliagnment -> Contant and non constant variables consistency adjusted, -> functions are rearranged as per ADO.Net Sample app

williammatherjones

Other than the tab/space issue, the sample looks really good. Please push a commit with the formatting changes and I'll approve.

williammatherjones · 2024-05-31T11:37:19Z

JDBC/OAuthSampleApp/src/main/java/OAuthSampleApp.java

+import com.google.gson.JsonObject;
+import com.vertica.jdbc.VerticaConnection;
+
+public class OAuthSampleApp {


I'm still seeing tabs in the PR. There is a setting in eclipse to use spaces instead of tabs when formatting code. If you have not enabled that, I suggest you change that and format the code again.

rvpanchal1202 added 18 commits April 17, 2024 09:37

Update OAuthSampleApp.java

c49ab5d

Commented SetUpDbForOAuth() and TearDown() call.

Update OAuthSampleApp.java

cb2a573

-> Added functions for the redundant code for copying data to the buffered output stream. -> removed unnecessary imports.

Update OAuthSampleApp.java

9be665f

Removed ClientID, ClientSecret, Tokenurl from getconnection Call.

Update OAuthSampleApp.java

43faac9

Added Line between imports and class

Update OAuthSampleApp.java

7f9f3fe

Debug prints removed

Update OAuthSampleApp.java

1f1dd1e

Deprecated method use JsonParser().parse(res); removed. used JsonParser.parseString(res);

Update OAuthSampleApp.java

43569a9

- Added Fix for setup of Authentication record to Database. - Uncommented DBSetup and Teardown for Authentication setup before OAuth sample test run

Update config.properties

e3fb283

Added DiscoveryUrl to be provided in config.properties file for OAuth Authentication

Update OAuthSampleApp.java

24678f1

Added RefreshToken check and If both the tokens are available, then it will use those tokens, Or else it will get new tokens using password grant.

Update OAuthSampleApp.java

e2b9639

Check if access token is not available, Get the tokens. else if a refresh token is not available, Get the tokens. else set both the tokens to properties.

Update OAuthSampleApp.java

bc955dd

Indentation issue resolved

Update OAuthSampleApp.java

222e8f2

Indentation resolved

Update OAuthSampleApp.java

d467034

Comment indentation resolved

Update OAuthSampleApp.java

19e7409

Internal review comments are addressed. -> Redundant code for getting parameters from connection string addressed. -> Formating of comments for method explanation addressed.

rvpanchal1202 marked this pull request as ready for review May 10, 2024 08:01

rvpanchal1202 added 2 commits May 10, 2024 15:07

Update OAuthSampleApp.java

26d633f

Review comments are addressed. Changes ensureAccessToken as per suggestion given Comments and print statements are added

Update OAuthSampleApp.java

932495e

-> Spacing between Functions and indentation formating is done

rvpanchal1202 mentioned this pull request May 10, 2024

[draft] Update OAuthSampleApp.java, Added refresh token api to get new access… #19

Closed

kevinkarch88 reviewed May 10, 2024

View reviewed changes

Update OAuthSampleApp.java

7241ce1

- Unused variable CONNECTION_STRING removed. - Added comments for the required calls - Added Refresh token check code in EnsureAccessToken Call

kevinkarch88 reviewed May 15, 2024

View reviewed changes

rvpanchal1202 added 3 commits May 16, 2024 17:01

Update OAuthSampleApp.java

e33d7a0

Refresh Token check is removed as it might be that we will get only access Token and not refresh token. As per comment from Kevin.

Update OAuthSampleApp.java

cac73b9

-> Updated comment for EnsureAccessToken

Update OAuthSampleApp.java

bdce33e

Changed the function name from connectToDb to getConnection.

kevinkarch88 reviewed May 16, 2024

View reviewed changes

JDBC/OAuthSampleApp/src/main/java/OAuthSampleApp.java Outdated Show resolved Hide resolved

Update OAuthSampleApp.java

58ec75a

Added null and empty checks for refresh token to save in the properties.

DMickens reviewed May 23, 2024

View reviewed changes

Update OAuthSampleApp.java

736b5e9

Review comments addressed, -> Code alliagnment -> Contant and non constant variables consistency adjusted, -> functions are rearranged as per ADO.Net Sample app

williammatherjones requested changes May 31, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

JDBC sample app to show use of Auth Tokens to connect the database and get the tokens from endpoints #23

JDBC sample app to show use of Auth Tokens to connect the database and get the tokens from endpoints #23

rvpanchal1202 commented May 10, 2024

kevinkarch88 commented May 22, 2024

DMickens May 23, 2024

rvpanchal1202 May 28, 2024

DMickens May 23, 2024

rvpanchal1202 May 28, 2024

DMickens May 28, 2024

rvpanchal1202 May 30, 2024 •

edited

Loading

williammatherjones May 31, 2024

DMickens May 23, 2024 •

edited

Loading

rvpanchal1202 May 28, 2024

DMickens May 23, 2024

rvpanchal1202 May 28, 2024

DMickens May 23, 2024

rvpanchal1202 May 28, 2024

DMickens May 23, 2024

rvpanchal1202 May 28, 2024

DMickens May 23, 2024

rvpanchal1202 May 28, 2024

DMickens May 23, 2024

rvpanchal1202 May 28, 2024

DMickens commented May 23, 2024

rvpanchal1202 commented May 28, 2024

williammatherjones left a comment

williammatherjones May 31, 2024

JDBC sample app to show use of Auth Tokens to connect the database and get the tokens from endpoints #23

Are you sure you want to change the base?

JDBC sample app to show use of Auth Tokens to connect the database and get the tokens from endpoints #23

Conversation

rvpanchal1202 commented May 10, 2024

kevinkarch88 commented May 22, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rvpanchal1202 May 30, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DMickens May 23, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DMickens commented May 23, 2024

rvpanchal1202 commented May 28, 2024

williammatherjones left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rvpanchal1202 May 30, 2024 •

edited

Loading

DMickens May 23, 2024 •

edited

Loading