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.

Sign up for GitHub

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

Jump to bottom

Introduce JDBC based persistence for SAML #6294

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Introduce JDBC based persistence for SAML #6294

wants to merge 1 commit into from
+1,959 −30

Conversation

Osara-B
Copy link
Contributor

@Osara-B Osara-B commented Jan 16, 2025

Proposed changes in this pull request

  • Introduce JDBC based persistence for SAML

@codecov Codecov
Copy link

codecov bot commented Jan 16, 2025 •
edited
Loading

Codecov Report

Attention: Patch coverage is 86.74699% with 44 lines in your changes missing coverage. Please review.

Project coverage is 46.10%. Comparing base (413c5f0) to head (7968925).
Report is 77 commits behind head on master.

Files with missing lines Patch % Lines
.../identity/core/model/SAMLSSOServiceProviderDO.java 61.53% 3 Missing and 32 partials ⚠️
...ty/core/dao/JDBCSAMLSSOServiceProviderDAOImpl.java 95.98% 5 Missing and 4 partials ⚠️
Additional details and impacted files 
@@             Coverage Diff              @@
##             master    #6294      +/-   ##
============================================
+ Coverage     45.96%   46.10%   +0.13%     
- Complexity    14586    14654      +68     
============================================
  Files          1662     1665       +3     
  Lines        104487   104818     +331     
  Branches      18354    18395      +41     
============================================
+ Hits          48031    48327     +296     
- Misses        49612    49615       +3     
- Partials       6844     6876      +32
Flag Coverage Δ
unit 29.44% <86.74%> (+0.24%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

darshanasbg
darshanasbg reviewed Jan 16, 2025
View reviewed changes
....core/src/main/java/org/wso2/carbon/identity/core/dao/JDBCSAMLSSOServiceProviderDAOImpl.java Outdated
namedPreparedStatement -> namedPreparedStatement.setInt(TENANT_ID, tenantId));

for (SAMLSSOServiceProviderDO serviceProviderDO : serviceProvidersList) {
addProperties(processGetServiceProviderId(serviceProviderDO.getIssuer(), tenantId), serviceProviderDO);
Copy link
Contributor

Choose a reason for hiding this comment

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

addProperties -> populateProperties

Copy link
Contributor

@darshanasbg darshanasbg Jan 16, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Can we populate this details executing only one query, rather executing many (=serviceProvidersList.size)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

addProperties -> populateProperties

changed in
Introduce JDBC based persistence for SAML

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Can we populate this details executing only one query, rather executing many (=serviceProvidersList.size)?

will work on it

@jenkins-is-staging
Copy link

PR builder started
Link: https://github.com/wso2/product-is/actions/runs/12816704713

@jenkins-is-staging
Copy link

PR builder completed
Link: https://github.com/wso2/product-is/actions/runs/12816704713
Status: failure

@jenkins-is-staging
Copy link

PR builder started
Link: https://github.com/wso2/product-is/actions/runs/12816704713

@jenkins-is-staging
Copy link

PR builder completed
Link: https://github.com/wso2/product-is/actions/runs/12816704713
Status: failure

@darshanasbg
Copy link
Contributor

PR builder completed Link: https://github.com/wso2/product-is/actions/runs/12816704713 Status: failure

 [INFO] Results:
 [INFO] 
 [ERROR] Failures: 
 [ERROR] RegistryMountTestCase.testClear:138->deleteApplication:307 Application deletion failed expected:<204> but was:<401>
 [ERROR] RegistryMountTestCase.testInit:129->createApplication:301 Error while creatin...
 [INFO] 
 [ERROR] Tests run: 3036, Failures: 2, Errors: 0, Skipped: 1

@Osara-B shall we verify this test locally?

UdeshAthukorala
UdeshAthukorala reviewed Jan 17, 2025
View reviewed changes
....core/src/test/java/org/wso2/carbon/identity/core/JDBCSAMLSSOServiceProviderManagerTest.java Outdated
samlSSOServiceProviderManager.addServiceProvider(sampleServiceProvider1, TENANT_ID);
assertTrue(samlSSOServiceProviderManager.removeServiceProvider(getIssuerWithQualifier(ISSUER1), TENANT_ID));
assertNull(samlSSOServiceProviderManager.getServiceProvider(getIssuerWithQualifier(ISSUER1), TENANT_ID));

Copy link
Contributor

Choose a reason for hiding this comment

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

Let's remove the unwanted empty line

Copy link
Contributor Author

Choose a reason for hiding this comment

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

resolved

....core/src/test/java/org/wso2/carbon/identity/core/JDBCSAMLSSOServiceProviderManagerTest.java Outdated

lenient().when(dataSource.getConnection()).thenReturn(spyConnection);
lenient().doNothing().when(spyConnection).close();

Copy link
Contributor

Choose a reason for hiding this comment

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

Let's remove the unwanted empty line

Copy link
Contributor Author

Choose a reason for hiding this comment

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

resolved in Introduce JDBC based persistence for SAML

@sonarqubecloud SonarQubeCloud
Copy link

Quality Gate Passed Quality Gate passed

Issues
12 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
2.7% Duplication on New Code

See analysis details on SonarQube Cloud

@Osara-B
Copy link
Contributor Author

Osara-B commented Jan 17, 2025

PR builder completed Link: https://github.com/wso2/product-is/actions/runs/12816704713 Status: failure

 [INFO] Results:
 [INFO] 
 [ERROR] Failures: 
 [ERROR] RegistryMountTestCase.testClear:138->deleteApplication:307 Application deletion failed expected:<204> but was:<401>
 [ERROR] RegistryMountTestCase.testInit:129->createApplication:301 Error while creatin...
 [INFO] 
 [ERROR] Tests run: 3036, Failures: 2, Errors: 0, Skipped: 1

@Osara-B shall we verify this test locally?

Tests are passing locally

Screenshot 2025-01-17 at 14 34 27 Screenshot 2025-01-17 at 14 38 19

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@UdeshAthukorala UdeshAthukorala UdeshAthukorala left review comments

@darshanasbg darshanasbg darshanasbg left review comments

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@Osara-B @jenkins-is-staging @darshanasbg @UdeshAthukorala