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 39.34426% with 37 lines in your changes missing coverage. Please review.

Project coverage is 40.08%. Comparing base (ed81688) to head (ce692d6).
Report is 132 commits behind head on master.

Files with missing lines Patch % Lines
...rce/mgt/dao/impl/APIResourceManagementDAOImpl.java 37.73% 31 Missing and 2 partials ⚠️
...nternal/APIResourceManagementServiceComponent.java 0.00% 4 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff              @@
##             master    #6002      +/-   ##
============================================
+ Coverage     39.62%   40.08%   +0.46%     
+ Complexity    14299    14183     -116     
============================================
  Files          1733     1736       +3     
  Lines        119948   117466    -2482     
  Branches      19808    19516     -292     
============================================
- Hits          47525    47086     -439     
+ Misses        65115    63129    -1986     
+ Partials       7308     7251      -57     
Flag Coverage Δ
unit 23.96% <39.34%> (+0.18%) ⬆️

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.

Comment on lines +1263 to +1264
return filterQueryBuilder.getFilterAttributeValue().get(1).contains("internal") ||
filterQueryBuilder.getFilterAttributeValue().get(1).contains("console");
Copy link
Contributor

Choose a reason for hiding this comment

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

Don't we have an existing constant for internal and console?

@@ -445,6 +475,18 @@ public List<Scope> getScopesByTenantId(Integer tenantId, List<ExpressionNode> ex
appendFilterQuery(expressionNodes, filterQueryBuilder, true);
String query = SQLConstants.GET_SCOPES_BY_TENANT_ID + filterQueryBuilder.getFilterQuery() +
SQLConstants.GET_SCOPES_BY_TENANT_ID_TAIL;
try {
if (OrganizationManagementUtil.isOrganization(tenantId) && !isInternalFiltering(filterQueryBuilder)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

According to the logic in isInternalFiltering method, internal_org.. scopes also get considered , is that means internal_org.. scopes can't be searched ?

@@ -49,6 +49,14 @@
description="Delete API resources"/>
</Scopes>
</APIResource>
<APIResource name="API Resource Management API" identifier="/o/api/server/v1/api-resources"
Copy link
Contributor

Choose a reason for hiding this comment

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

shall we update the xml files as well for consistency

@@ -65,10 +65,18 @@ public class SQLConstants {
"DESCRIPTION, TENANT_ID, TYPE, REQUIRES_AUTHORIZATION FROM API_RESOURCE WHERE ";
public static final String GET_API_RESOURCES_TAIL =
" (TENANT_ID = %d OR TENANT_ID IS NULL) ORDER BY CURSOR_KEY %s LIMIT %d";
public static final String GET_API_RESOURCES_TAIL_FOR_ORGANIZATIONS =
" (TENANT_ID = %d OR TENANT_ID IS NULL) AND TYPE != 'TENANT' AND 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.

Shall we use the equal conditions for types rather than not equal conditions because if we add another type for root organization those will be reflected in organization API responses.

Copy link
Contributor

@AnuradhaSK AnuradhaSK Oct 15, 2024

Choose a reason for hiding this comment

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

Also can we go for this pattern for more readability TYPE NOT IN ('TENANT', 'SYSTEM') or
TYPE IN ('<X>', '<Y>')

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.

3 participants