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.

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

Enable API Resources APIs for organization level #6002

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

Conversation

ShanChathusanda93
Copy link
Contributor

Proposed changes in this pull request

Follow up actions

@ShanChathusanda93 ShanChathusanda93 force-pushed the org-path-api-res-branch branch 2 times, most recently from 8a510db to 39fa1e2 Compare October 9, 2024 06:39
Copy link

codecov bot commented Oct 9, 2024

Codecov Report

Attention: Patch coverage is 27.77778% with 65 lines in your changes missing coverage. Please review.

Project coverage is 40.37%. Comparing base (565230f) to head (13aa92b).
Report is 151 commits behind head on master.

Files with missing lines Patch % Lines
...rce/mgt/dao/impl/APIResourceManagementDAOImpl.java 25.60% 59 Missing and 2 partials ⚠️
...nternal/APIResourceManagementServiceComponent.java 0.00% 4 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff              @@
##             master    #6002      +/-   ##
============================================
- Coverage     40.43%   40.37%   -0.06%     
+ Complexity    14687    14324     -363     
============================================
  Files          1746     1746              
  Lines        123017   117515    -5502     
  Branches      21841    19087    -2754     
============================================
- Hits          49736    47444    -2292     
+ Misses        65716    62792    -2924     
+ Partials       7565     7279     -286     
Flag Coverage Δ
unit 24.48% <27.77%> (+<0.01%) ⬆️

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.

@ShanChathusanda93 ShanChathusanda93 force-pushed the org-path-api-res-branch branch 2 times, most recently from 1650b88 to 853e708 Compare October 21, 2024 05:39
Comment on lines 126 to 139
public static final String GET_API_RESOURCE_BY_ID_FOR_ORGANIZATIONS = "SELECT" +
" AR.ID AS API_RESOURCE_ID," +
" AR.NAME AS API_RESOURCE_NAME," +
" AR.IDENTIFIER AS API_RESOURCE_IDENTIFIER," +
" AR.DESCRIPTION AS API_RESOURCE_DESCRIPTION," +
" AR.TENANT_ID AS API_RESOURCE_TENANT_ID," +
" AR.TYPE AS API_RESOURCE_TYPE," +
" AR.REQUIRES_AUTHORIZATION AS REQUIRES_AUTHORIZATION," +
" S.ID AS SCOPE_ID," +
" S.NAME AS SCOPE_QUALIFIED_NAME," +
" S.DISPLAY_NAME AS SCOPE_DISPLAY_NAME," +
" S.DESCRIPTION AS SCOPE_DESCRIPTION" +
" FROM API_RESOURCE AR LEFT JOIN SCOPE S ON AR.ID = S.API_ID WHERE AR.ID = ? AND (AR.TENANT_ID = ?" +
" OR AR.TENANT_ID IS NULL) AND AR.TYPE != 'TENANT' AND AR.TYPE != 'SYSTEM'";
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do't we have DB-specific select queries. Does this query works on all of our supported DBs?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As of now this was tested against H2, MySQL and MSSQL. For those this is working as expected. Other DB type testings are in progress.

Comment on lines 87 to 94
{TENANT_ID, new ArrayList<>(), false, 2},
{INVALID_TENANT_ID, new ArrayList<>(), true, 0},
Copy link
Contributor

Choose a reason for hiding this comment

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

Here the combinations are less.

We need to execute the test for correct TENANT_ID and INVALID_TENANT_ID when isOrganization is false
TENANT_ID and INVALID_TENANT_ID when isOrganization is true

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do we need to test the INVALID_TENANT_ID when isOrganization is true?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added a new combination

Comment on lines +191 to +205
String apiId = addAPIResourceToDB(name, getConnection(), tenantId, identityDatabaseUtil,
organizationManagementUtil, isOrganization).getId();
Copy link
Contributor

Choose a reason for hiding this comment

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

we are not allowing to add API resources for sub orgs right?
As per this test, you are adding the API resources to the sub org tenant id

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No we are not allowing to create the API resources in sub org level. Here the isOrganization property is sending to the addAPIResourceToDB is for the mock purposes.

image

Mocking is added since in the actual DAO method to add the API resource, it will retrieve the added API resource from the get method where we are checking whether the corresponding tenant is an organization or not.

@@ -529,6 +576,7 @@ private APIResource addAPIResourceToDB(String namePostFix, Connection connection
connection.commit();
return null;
});
organizationManagementUtil.when(() -> OrganizationManagementUtil.isOrganization(anyInt())).thenReturn(false);
Copy link
Contributor

Choose a reason for hiding this comment

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

Why this mocking is required? can't find a usage of this in the DAO impl of addAPIResource method

Copy link
Contributor Author

Choose a reason for hiding this comment

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

When adding the API resource, inside the DAO method, it will extract the added API resource from the id[1]. Inside that method we added the organization checkl and this mocking is needed for that.

image

[1]

@@ -243,16 +351,32 @@ public void testIsScopeExistById(Integer tenantId, boolean expected) throws Exce
public Object[][] deleteAPIResourceByIdData() {

return new Object[][]{
{TENANT_ID, false}
{TENANT_ID, false, false},
{TENANT_ID, true, false}
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this test correct? in sub org level to we store API resource?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No we dont. Here what the test is adding a resource and deleting that. The isOrganization property is for mocking purposes.

@ShanChathusanda93 ShanChathusanda93 force-pushed the org-path-api-res-branch branch 4 times, most recently from 7bbfc39 to 8c3c429 Compare October 24, 2024 09:53
Copy link

sonarcloud bot commented Oct 25, 2024

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.

4 participants