Add DevPortal Governance backend templates and enforcement#13832
Open
ashiduDissanayake wants to merge 4 commits into
Open
Add DevPortal Governance backend templates and enforcement#13832ashiduDissanayake wants to merge 4 commits into
ashiduDissanayake wants to merge 4 commits into
Conversation
ashiduDissanayake
requested review from
AnuGayan,
Arshardh,
HeshanSudarshana,
HiranyaKavishani,
Krishanx92,
PasanT9,
RakhithaRR,
ashera96,
chamilaadhi,
dushaniw,
hisanhunais,
npamudika,
pubudu538,
senthuran16,
tgtshanika,
tharikaGitHub and
tharindu1st
as code owners
|
|
📝 WalkthroughWalkthroughAdds Devportal governance templates: API contracts/models/enums, governance manager/validator and KM context, DAO/SQL and snapshots, REST endpoints/specs, Store enforcement/defaulting, admin/common spec updates, tenant scopes, and default Spectral rulesets. ChangesDevportal templates, validation, and Store integration
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes ✨ Finishing Touches🧪 Generate unit tests (beta)
|
![wso2-engineering[bot]](https://avatars.githubusercontent.com/in/1856828?s=60&v=4){kind=small}
Comment on lines
+109
to
+116
|
|
||
| public void setFormConfig(Map<String, Object> formConfig) { | ||
|
|
||
| if (formConfig == null) { | ||
| this.formConfig = Collections.emptyMap(); | ||
| } else { | ||
| this.formConfig = Collections.unmodifiableMap(new HashMap<>(formConfig)); | ||
| } |
Contributor
There was a problem hiding this comment.
Log Improvement Suggestion No: 1
Suggested change
| public void setFormConfig(Map<String, Object> formConfig) { | |
| if (formConfig == null) { | |
| this.formConfig = Collections.emptyMap(); | |
| } else { | |
| this.formConfig = Collections.unmodifiableMap(new HashMap<>(formConfig)); | |
| } | |
| public void setFormConfig(Map<String, Object> formConfig) { | |
| if (formConfig == null) { | |
| log.debug("Setting form config to empty map as input was null"); | |
| this.formConfig = Collections.emptyMap(); | |
| } else { | |
| log.debug("Setting form config with " + formConfig.size() + " entries"); | |
| this.formConfig = Collections.unmodifiableMap(new HashMap<>(formConfig)); | |
| } | |
| } |
Comment on lines
+163
to
+170
|
|
||
| public void setRulesetSnapshots(List<DevportalGovernanceRulesetSnapshot> rulesetSnapshots) { | ||
|
|
||
| if (rulesetSnapshots == null) { | ||
| this.rulesetSnapshots = Collections.emptyList(); | ||
| } else { | ||
| this.rulesetSnapshots = Collections.unmodifiableList(new ArrayList<>(rulesetSnapshots)); | ||
| } |
Contributor
There was a problem hiding this comment.
Log Improvement Suggestion No: 2
Suggested change
| public void setRulesetSnapshots(List<DevportalGovernanceRulesetSnapshot> rulesetSnapshots) { | |
| if (rulesetSnapshots == null) { | |
| this.rulesetSnapshots = Collections.emptyList(); | |
| } else { | |
| this.rulesetSnapshots = Collections.unmodifiableList(new ArrayList<>(rulesetSnapshots)); | |
| } | |
| public void setRulesetSnapshots(List<DevportalGovernanceRulesetSnapshot> rulesetSnapshots) { | |
| if (rulesetSnapshots == null) { | |
| log.debug("Setting ruleset snapshots to empty list as input was null"); | |
| this.rulesetSnapshots = Collections.emptyList(); | |
| } else { | |
| log.debug("Setting " + rulesetSnapshots.size() + " ruleset snapshots"); | |
| this.rulesetSnapshots = Collections.unmodifiableList(new ArrayList<>(rulesetSnapshots)); | |
| } | |
| } |
Comment on lines
+106
to
+113
|
|
||
| public void setFormConfig(Map<String, Object> formConfig) { | ||
|
|
||
| if (formConfig == null) { | ||
| this.formConfig = Collections.emptyMap(); | ||
| } else { | ||
| this.formConfig = Collections.unmodifiableMap(new HashMap<>(formConfig)); | ||
| } |
Contributor
There was a problem hiding this comment.
Log Improvement Suggestion No: 3
Suggested change
| public void setFormConfig(Map<String, Object> formConfig) { | |
| if (formConfig == null) { | |
| this.formConfig = Collections.emptyMap(); | |
| } else { | |
| this.formConfig = Collections.unmodifiableMap(new HashMap<>(formConfig)); | |
| } | |
| public void setFormConfig(Map<String, Object> formConfig) { | |
| if (formConfig == null) { | |
| log.debug("FormConfig is null, setting to empty map"); | |
| this.formConfig = Collections.emptyMap(); | |
| } else { | |
| log.debug("Setting formConfig with {} entries", formConfig.size()); | |
| this.formConfig = Collections.unmodifiableMap(new HashMap<>(formConfig)); | |
| } |
Comment on lines
+211
to
+218
| public void setRulesetBindings(List<DevportalGovernanceRulesetBinding> rulesetBindings) { | ||
|
|
||
| if (rulesetBindings == null) { | ||
| this.rulesetBindings = Collections.emptyList(); | ||
| } else { | ||
| this.rulesetBindings = Collections.unmodifiableList(new ArrayList<>(rulesetBindings)); | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
Log Improvement Suggestion No: 4
Suggested change
| public void setRulesetBindings(List<DevportalGovernanceRulesetBinding> rulesetBindings) { | |
| if (rulesetBindings == null) { | |
| this.rulesetBindings = Collections.emptyList(); | |
| } else { | |
| this.rulesetBindings = Collections.unmodifiableList(new ArrayList<>(rulesetBindings)); | |
| } | |
| } | |
| public void setRulesetBindings(List<DevportalGovernanceRulesetBinding> rulesetBindings) { | |
| if (rulesetBindings == null) { | |
| log.debug("RulesetBindings is null, setting to empty list"); | |
| this.rulesetBindings = Collections.emptyList(); | |
| } else { | |
| log.debug("Setting rulesetBindings with {} entries", rulesetBindings.size()); | |
| this.rulesetBindings = Collections.unmodifiableList(new ArrayList<>(rulesetBindings)); | |
| } |
Comment on lines
+47
to
+54
|
|
||
| public void setTemplateList(List<DevportalGovernanceTemplate> templateList) { | ||
|
|
||
| if (templateList == null) { | ||
| this.templateList = Collections.emptyList(); | ||
| } else { | ||
| this.templateList = Collections.unmodifiableList(new ArrayList<>(templateList)); | ||
| } |
Contributor
There was a problem hiding this comment.
Log Improvement Suggestion No: 5
Suggested change
| public void setTemplateList(List<DevportalGovernanceTemplate> templateList) { | |
| if (templateList == null) { | |
| this.templateList = Collections.emptyList(); | |
| } else { | |
| this.templateList = Collections.unmodifiableList(new ArrayList<>(templateList)); | |
| } | |
| public void setTemplateList(List<DevportalGovernanceTemplate> templateList) { | |
| if (templateList == null) { | |
| log.debug("Setting empty template list as null was provided"); | |
| this.templateList = Collections.emptyList(); | |
| } else { | |
| log.debug("Setting template list with size: " + templateList.size()); | |
| this.templateList = Collections.unmodifiableList(new ArrayList<>(templateList)); | |
| } |
Comment on lines
34
to
35
| public static RuleType fromString(String ruleTypeString) { | ||
| try { |
Contributor
There was a problem hiding this comment.
Log Improvement Suggestion No: 6
Suggested change
| public static RuleType fromString(String ruleTypeString) { | |
| try { | |
| public static RuleType fromString(String ruleTypeString) { | |
| try { | |
| log.debug("Converting string '{}' to RuleType", ruleTypeString); |
Comment on lines
+91
to
+94
|
|
||
| @Override | ||
| public boolean isArtifactAvailable(String artifactRefId, String organization) throws APIMGovernanceException { | ||
| try { |
Contributor
There was a problem hiding this comment.
Log Improvement Suggestion No: 7
Suggested change
| @Override | |
| public boolean isArtifactAvailable(String artifactRefId, String organization) throws APIMGovernanceException { | |
| try { | |
| @Override | |
| public boolean isArtifactAvailable(String artifactRefId, String organization) throws APIMGovernanceException { | |
| log.info("Checking availability of application: " + artifactRefId); | |
| try { |
Comment on lines
+189
to
+194
| return ExtendedArtifactType.APPLICATION; | ||
| } | ||
|
|
||
| private Application getApplication(String uuid) throws APIMGovernanceException { | ||
| try { | ||
| Application application = ApiMgtDAO.getInstance().getApplicationByUUID(uuid); |
Contributor
There was a problem hiding this comment.
Log Improvement Suggestion No: 8
Suggested change
| return ExtendedArtifactType.APPLICATION; | |
| } | |
| private Application getApplication(String uuid) throws APIMGovernanceException { | |
| try { | |
| Application application = ApiMgtDAO.getInstance().getApplicationByUUID(uuid); | |
| private Application getApplication(String uuid) throws APIMGovernanceException { | |
| if (log.isDebugEnabled()) { | |
| log.debug("Retrieving application with UUID: " + uuid); | |
| } | |
| try { | |
| Application application = ApiMgtDAO.getInstance().getApplicationByUUID(uuid); | |
| if (application == null) { | |
| log.error("Application not found with UUID: " + uuid); | |
| throw new APIMGovernanceException(APIMGovExceptionCodes.ERROR_WHILE_GETTING_APPLICATION_INFO, uuid); | |
| } |
Comment on lines
+61
to
+63
| if (ArtifactType.APPLICATION.equals(artifactType)) { | ||
| return new ApplicationGovernanceHandler(); | ||
| } |
Contributor
There was a problem hiding this comment.
Log Improvement Suggestion No: 9
Suggested change
| if (ArtifactType.APPLICATION.equals(artifactType)) { | |
| return new ApplicationGovernanceHandler(); | |
| } | |
| if (ArtifactType.APPLICATION.equals(artifactType)) { | |
| log.debug("Creating ApplicationGovernanceHandler for artifact type: {}", artifactType); | |
| return new ApplicationGovernanceHandler(); | |
| } |
Comment on lines
+91
to
+93
| template.setOrganization(organization); | ||
| if (isBlank(template.getStatus())) { | ||
| template.setStatus(DevportalGovernanceTemplate.STATUS_DRAFT); |
Contributor
There was a problem hiding this comment.
Log Improvement Suggestion No: 10
Suggested change
| template.setOrganization(organization); | |
| if (isBlank(template.getStatus())) { | |
| template.setStatus(DevportalGovernanceTemplate.STATUS_DRAFT); | |
| public DevportalGovernanceTemplate createTemplate(DevportalGovernanceTemplate template, String organization) | |
| throws APIMGovernanceException { | |
| log.info("Creating Devportal Governance template: {} for organization: {}", template.getName(), organization); |
Comment on lines
+151
to
+152
| * @param templateId template ID | ||
| * @param username username |
Contributor
There was a problem hiding this comment.
Log Improvement Suggestion No: 11
Suggested change
| * @param templateId template ID | |
| * @param username username | |
| DevportalGovernanceTemplate updatedTemplate = | |
| devportalGovernanceDAO.updateTemplate(templateId, template, organization); | |
| log.info("Successfully updated Devportal Governance template: {} with id: {} in organization: {}", updatedTemplate.getName(), templateId, organization); |
Comment on lines
+63
to
+65
| */ | ||
| public static void synchronizeAllTemplates() { | ||
|
|
Contributor
There was a problem hiding this comment.
Log Improvement Suggestion No: 12
Suggested change
| */ | |
| public static void synchronizeAllTemplates() { | |
| public static void synchronizeAllTemplates() { | |
| log.info("Starting synchronization of Devportal Governance template server-backed fields"); | |
| DevportalGovernanceDAO dao = DevportalGovernanceDAO.getInstance(); |
Comment on lines
+82
to
+88
| updatedCount++; | ||
| } | ||
| } | ||
| if (log.isDebugEnabled()) { | ||
| log.debug("Synchronized Devportal Governance template server-backed fields. Updated templates: " | ||
| + updatedCount); | ||
| } |
Contributor
There was a problem hiding this comment.
Log Improvement Suggestion No: 13
Suggested change
| updatedCount++; | |
| } | |
| } | |
| if (log.isDebugEnabled()) { | |
| log.debug("Synchronized Devportal Governance template server-backed fields. Updated templates: " | |
| + updatedCount); | |
| } | |
| } | |
| if (log.isDebugEnabled()) { | |
| log.debug("Synchronized Devportal Governance template server-backed fields. Updated templates: " | |
| + updatedCount); | |
| } | |
| log.info("Completed synchronization of Devportal Governance templates. Total updated: " + updatedCount); | |
| } catch (APIMGovernanceException e) { |
Comment on lines
+40
to
+42
| */ | ||
| public class DevportalGovernanceValidator { | ||
|
|
Contributor
There was a problem hiding this comment.
Log Improvement Suggestion No: 14
Suggested change
| */ | |
| public class DevportalGovernanceValidator { | |
| public class DevportalGovernanceValidator { | |
| private static final Log log = LogFactory.getLog(DevportalGovernanceValidator.class); | |
| /** |
Comment on lines
+80
to
+81
| } | ||
| ValidationEngine validationEngine = getValidationEngine(); |
Contributor
There was a problem hiding this comment.
Log Improvement Suggestion No: 15
Suggested change
| } | |
| ValidationEngine validationEngine = getValidationEngine(); | |
| for (DevportalGovernanceRulesetSnapshot rulesetSnapshot : rulesetSnapshots) { | |
| if (log.isDebugEnabled()) { | |
| log.debug("Validating against ruleset: " + rulesetSnapshot.getRulesetName() + | |
| " for application: " + applicationUuid); | |
| } | |
| Ruleset ruleset = toRuleset(rulesetSnapshot); |
Comment on lines
+100
to
+101
| } else if (isRulesetAssociatedWithTemplates(rulesetId, organization)) { | ||
| throw new APIMGovernanceException(APIMGovExceptionCodes.ERROR_RULESET_ASSOCIATED_WITH_TEMPLATES, |
Contributor
There was a problem hiding this comment.
Log Improvement Suggestion No: 16
Suggested change
| } else if (isRulesetAssociatedWithTemplates(rulesetId, organization)) { | |
| throw new APIMGovernanceException(APIMGovExceptionCodes.ERROR_RULESET_ASSOCIATED_WITH_TEMPLATES, | |
| } else if (isRulesetAssociatedWithTemplates(rulesetId, organization)) { | |
| log.warn("Attempted to delete ruleset {} that is associated with templates in organization {}", rulesetId, organization); | |
| throw new APIMGovernanceException(APIMGovExceptionCodes.ERROR_RULESET_ASSOCIATED_WITH_TEMPLATES, |
Comment on lines
+130
to
+131
| throws APIMGovernanceException { | ||
| List<String> templateIds = rulesetMgtDAO.getAssociatedTemplatesForRuleset(rulesetId, organization); |
Contributor
There was a problem hiding this comment.
Log Improvement Suggestion No: 17
Suggested change
| throws APIMGovernanceException { | |
| List<String> templateIds = rulesetMgtDAO.getAssociatedTemplatesForRuleset(rulesetId, organization); | |
| throws APIMGovernanceException { | |
| if (log.isDebugEnabled()) { | |
| log.debug("Checking if ruleset {} is associated with any templates in organization {}", rulesetId, organization); | |
| } | |
| List<String> templateIds = rulesetMgtDAO.getAssociatedTemplatesForRuleset(rulesetId, organization); |
Comment on lines
+88
to
+90
| throws APIMGovernanceException { | ||
|
|
||
| try (Connection connection = APIMgtDBUtil.getConnection()) { |
Contributor
There was a problem hiding this comment.
Log Improvement Suggestion No: 18
Suggested change
| throws APIMGovernanceException { | |
| try (Connection connection = APIMgtDBUtil.getConnection()) { | |
| public DevportalGovernanceTemplate createTemplate(DevportalGovernanceTemplate template, String organization) | |
| throws APIMGovernanceException { | |
| log.info("Creating Devportal Governance template: " + template.getName() + " in organization: " + organization); |
Comment on lines
+101
to
+102
| return getTemplateById(template.getId(), organization); | ||
| } catch (JsonProcessingException | NoSuchAlgorithmException | SQLException e) { |
Contributor
There was a problem hiding this comment.
Log Improvement Suggestion No: 19
Suggested change
| return getTemplateById(template.getId(), organization); | |
| } catch (JsonProcessingException | NoSuchAlgorithmException | SQLException e) { | |
| connection.commit(); | |
| log.info("Successfully created Devportal Governance template: " + template.getName() + " with ID: " + template.getId()); | |
| return getTemplateById(template.getId(), organization); |
Comment on lines
+593
to
+596
| @Override | ||
| public List<String> getAssociatedTemplatesForRuleset(String rulesetId, String organization) | ||
| throws APIMGovernanceException { | ||
| List<String> templateIds = new ArrayList<>(); |
Contributor
There was a problem hiding this comment.
Log Improvement Suggestion No: 20
Suggested change
| @Override | |
| public List<String> getAssociatedTemplatesForRuleset(String rulesetId, String organization) | |
| throws APIMGovernanceException { | |
| List<String> templateIds = new ArrayList<>(); | |
| @Override | |
| public List<String> getAssociatedTemplatesForRuleset(String rulesetId, String organization) | |
| throws APIMGovernanceException { | |
| log.debug("Fetching associated templates for ruleset: {} in organization: {}", rulesetId, organization); | |
| List<String> templateIds = new ArrayList<>(); |
Comment on lines
+606
to
+608
| } | ||
| } catch (SQLException e) { | ||
| throw new APIMGovernanceException(APIMGovExceptionCodes.ERROR_WHILE_GETTING_ASSOCIATED_TEMPLATES, |
Contributor
There was a problem hiding this comment.
Log Improvement Suggestion No: 21
Suggested change
| } | |
| } catch (SQLException e) { | |
| throw new APIMGovernanceException(APIMGovExceptionCodes.ERROR_WHILE_GETTING_ASSOCIATED_TEMPLATES, | |
| } | |
| } catch (SQLException e) { | |
| log.error("Error while getting associated templates for ruleset: {}", rulesetId); | |
| throw new APIMGovernanceException(APIMGovExceptionCodes.ERROR_WHILE_GETTING_ASSOCIATED_TEMPLATES, |
Comment on lines
68
to
+69
| APIMGovernanceUtil.loadDefaultPolicies(MultitenantConstants.SUPER_TENANT_DOMAIN_NAME); | ||
| DevportalGovernanceTemplateConfigSynchronizer.synchronizeAllTemplates(); |
Contributor
There was a problem hiding this comment.
Log Improvement Suggestion No: 22
Suggested change
| APIMGovernanceUtil.loadDefaultPolicies(MultitenantConstants.SUPER_TENANT_DOMAIN_NAME); | |
| DevportalGovernanceTemplateConfigSynchronizer.synchronizeAllTemplates(); | |
| APIMGovernanceUtil.loadDefaultPolicies(MultitenantConstants.SUPER_TENANT_DOMAIN_NAME); | |
| log.info("Starting synchronization of all devportal governance templates"); | |
| DevportalGovernanceTemplateConfigSynchronizer.synchronizeAllTemplates(); |
Comment on lines
+62
to
+64
| */ | ||
| public static KeyManagerGovernanceContext resolve(String identifier, String organization) { | ||
|
|
Contributor
There was a problem hiding this comment.
Log Improvement Suggestion No: 23
Suggested change
| */ | |
| public static KeyManagerGovernanceContext resolve(String identifier, String organization) { | |
| public static KeyManagerGovernanceContext resolve(String identifier, String organization) { | |
| log.info("Resolving KeyManager context for organization: {}", organization); | |
| KeyManagerConfigurationDTO config = lookup(identifier, organization); |
Comment on lines
+86
to
+88
| return null; | ||
| } | ||
|
|
Contributor
There was a problem hiding this comment.
Log Improvement Suggestion No: 24
Suggested change
| return null; | |
| } | |
| private static KeyManagerConfigurationDTO lookupInOrganization(String identifier, String organization) { | |
| log.debug("Looking up KeyManager '{}' in organization '{}'", identifier, organization); | |
| ApiMgtDAO dao = ApiMgtDAO.getInstance(); |
Contributor
There was a problem hiding this comment.
AI Agent Log Improvement Checklist
- The log-related comments and suggestions in this review were generated by an AI tool to assist with identifying potential improvements. Purpose of reviewing the code for log improvements is to improve the troubleshooting capabilities of our products.
- Please make sure to manually review and validate all suggestions before applying any changes. Not every code suggestion would make sense or add value to our purpose. Therefore, you have the freedom to decide which of the suggestions are helpful.
✅ Before merging this pull request:
- Review all AI-generated comments for accuracy and relevance.
- Complete and verify the table below. We need your feedback to measure the accuracy of these suggestions and the value they add. If you are rejecting a certain code suggestion, please mention the reason briefly in the suggestion for us to capture it.
Contributor
There was a problem hiding this comment.
Pull request overview
This PR introduces the backend foundation for DevPortal Governance in APIM, including admin-managed governance templates, application snapshotting, new REST APIs, and synchronous enforcement hooks for application metadata and OAuth client/key lifecycle operations.
Changes:
- Added DevPortal Governance template model + persistence + REST API (including default template resolution and hidden-default validation support).
- Added application governance snapshot storage and request-time enforcement hooks for application create/update and OAuth lifecycle operations.
- Extended governance domain models/enums to support APPLICATION artifacts and APP_INFO / APP_OAUTH rule types, plus default baseline rulesets.
Reviewed changes
Copilot reviewed 67 out of 67 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| features/apimgt/org.wso2.carbon.apimgt.governance.feature/src/main/resources/default-rulesets/wso2_oauth_key_governance_baseline.yaml | Adds baseline APP_OAUTH Spectral ruleset for OAuth key/client lifecycle governance. |
| features/apimgt/org.wso2.carbon.apimgt.governance.feature/src/main/resources/default-rulesets/wso2_devportal_application_baseline.yaml | Adds baseline APP_INFO Spectral ruleset for DevPortal application metadata hygiene. |
| features/apimgt/org.wso2.carbon.apimgt.core.feature/src/main/resources/sql/postgresql.sql | Adds DevPortal template + ruleset binding + app snapshot tables (PostgreSQL). |
| features/apimgt/org.wso2.carbon.apimgt.core.feature/src/main/resources/sql/oracle.sql | Adds DevPortal template + ruleset binding + app snapshot tables (Oracle). |
| features/apimgt/org.wso2.carbon.apimgt.core.feature/src/main/resources/sql/mysql.sql | Adds DevPortal template + ruleset binding + app snapshot tables (MySQL). |
| features/apimgt/org.wso2.carbon.apimgt.core.feature/src/main/resources/sql/mssql.sql | Adds DevPortal template + ruleset binding + app snapshot tables (MSSQL). |
| features/apimgt/org.wso2.carbon.apimgt.core.feature/src/main/resources/sql/h2.sql | Adds DevPortal template + ruleset binding + app snapshot tables (H2). |
| components/apimgt/org.wso2.carbon.apimgt.rest.api.util/src/main/resources/devportal-api.yaml | Adds templateId to application schemas; fixes YAML formatting. |
| components/apimgt/org.wso2.carbon.apimgt.rest.api.store.v1/src/main/resources/devportal-api.yaml | Adds templateId/governanceFormConfig and relaxes required fields for template-default flows. |
| components/apimgt/org.wso2.carbon.apimgt.rest.api.store.v1/src/main/java/org/wso2/carbon/apimgt/rest/api/store/v1/impl/SubscriptionsApiServiceImpl.java | Removes subscription-level governance invocation (keeps compatibility note). |
| components/apimgt/org.wso2.carbon.apimgt.rest.api.store.v1/src/main/java/org/wso2/carbon/apimgt/rest/api/store/v1/impl/ApplicationsApiServiceImpl.java | Adds template defaults application, validation, snapshot capture, and OAuth lifecycle governance gates. |
| components/apimgt/org.wso2.carbon.apimgt.rest.api.store.v1/src/gen/java/org/wso2/carbon/apimgt/rest/api/store/v1/dto/SubscriptionDTO.java | Updates generated DTO validation annotations to reflect API spec changes. |
| components/apimgt/org.wso2.carbon.apimgt.rest.api.store.v1/src/gen/java/org/wso2/carbon/apimgt/rest/api/store/v1/dto/ApplicationKeyGenerateRequestDTO.java | Updates generated DTO validation annotations to reflect API spec changes. |
| components/apimgt/org.wso2.carbon.apimgt.rest.api.store.v1/src/gen/java/org/wso2/carbon/apimgt/rest/api/store/v1/dto/ApplicationInfoDTO.java | Adds templateId field to application info DTO. |
| components/apimgt/org.wso2.carbon.apimgt.rest.api.store.v1/src/gen/java/org/wso2/carbon/apimgt/rest/api/store/v1/dto/ApplicationDTO.java | Adds templateId and governanceFormConfig to application DTO; adjusts throttlingPolicy requirement. |
| components/apimgt/org.wso2.carbon.apimgt.rest.api.store.v1/pom.xml | Adds provided-scope dependencies for governance api/impl to store REST API module. |
| components/apimgt/org.wso2.carbon.apimgt.rest.api.common/src/main/resources/devportal-api.yaml | Mirrors Store DevPortal OpenAPI changes (template + defaults + scopes). |
| components/apimgt/org.wso2.carbon.apimgt.rest.api.common/src/main/resources/admin-api.yaml | Adds admin settings fields and governance template scopes. |
| components/apimgt/org.wso2.carbon.apimgt.rest.api.admin.v1/src/main/resources/admin-api.yaml | Mirrors admin OpenAPI changes for settings + template scopes. |
| components/apimgt/org.wso2.carbon.apimgt.rest.api.admin.v1/src/main/java/org/wso2/carbon/apimgt/rest/api/admin/v1/utils/mappings/SettingsMappingUtil.java | Adds applicationSharingEnabled field mapping. |
| components/apimgt/org.wso2.carbon.apimgt.rest.api.admin.v1/src/main/java/org/wso2/carbon/apimgt/rest/api/admin/v1/impl/SettingsApiServiceImpl.java | Adds application attribute config data into admin settings response. |
| components/apimgt/org.wso2.carbon.apimgt.rest.api.admin.v1/src/gen/java/org/wso2/carbon/apimgt/rest/api/admin/v1/dto/SettingsDTO.java | Adds new settings response fields for app sharing + attributes. |
| components/apimgt/org.wso2.carbon.apimgt.impl/src/main/resources/tenant/tenant-conf.json | Adds new OAuth scopes for governance template read/manage. |
| components/apimgt/org.wso2.carbon.apimgt.governance.rest.api/src/main/webapp/WEB-INF/web.xml | Registers Templates API in governance REST webapp. |
| components/apimgt/org.wso2.carbon.apimgt.governance.rest.api/src/main/webapp/WEB-INF/beans.xml | Registers Templates API bean. |
| components/apimgt/org.wso2.carbon.apimgt.governance.rest.api/src/main/java/org/wso2/carbon/apimgt/governance/rest/api/mappings/TemplateMappingUtil.java | Adds model/DTO mapping utilities for templates + bindings + KM scopes. |
| components/apimgt/org.wso2.carbon.apimgt.governance.rest.api/src/main/java/org/wso2/carbon/apimgt/governance/rest/api/impl/TemplatesApiServiceImpl.java | Implements template CRUD/list/default + validate-defaults endpoints. |
| components/apimgt/org.wso2.carbon.apimgt.governance.rest.api/src/gen/java/org/wso2/carbon/apimgt/governance/rest/api/TemplatesApiService.java | Generated Templates API service interface. |
| components/apimgt/org.wso2.carbon.apimgt.governance.rest.api/src/gen/java/org/wso2/carbon/apimgt/governance/rest/api/TemplatesApi.java | Generated Templates API JAX-RS resource. |
| components/apimgt/org.wso2.carbon.apimgt.governance.rest.api/src/gen/java/org/wso2/carbon/apimgt/governance/rest/api/dto/TemplateDefaultViolationListDTO.java | DTO for validate-defaults response envelope. |
| components/apimgt/org.wso2.carbon.apimgt.governance.rest.api/src/gen/java/org/wso2/carbon/apimgt/governance/rest/api/dto/TemplateDefaultViolationDTO.java | DTO for validate-defaults individual rule violations. |
| components/apimgt/org.wso2.carbon.apimgt.governance.rest.api/src/gen/java/org/wso2/carbon/apimgt/governance/rest/api/dto/RulesetInputDTO.java | Extends rule/artifact enums to include application governance types. |
| components/apimgt/org.wso2.carbon.apimgt.governance.rest.api/src/gen/java/org/wso2/carbon/apimgt/governance/rest/api/dto/RulesetInfoDTO.java | Extends rule/artifact enums to include application governance types. |
| components/apimgt/org.wso2.carbon.apimgt.governance.rest.api/src/gen/java/org/wso2/carbon/apimgt/governance/rest/api/dto/DevportalGovernanceTemplateListDTO.java | DTO for listing templates with pagination. |
| components/apimgt/org.wso2.carbon.apimgt.governance.rest.api/src/gen/java/org/wso2/carbon/apimgt/governance/rest/api/dto/DevportalGovernanceTemplateDTO.java | DTO for template details (formConfig, defaults, bindings). |
| components/apimgt/org.wso2.carbon.apimgt.governance.rest.api/src/gen/java/org/wso2/carbon/apimgt/governance/rest/api/dto/DevportalGovernanceRulesetBindingDTO.java | DTO for template↔ruleset binding (incl. KM scopes). |
| components/apimgt/org.wso2.carbon.apimgt.governance.rest.api/src/gen/java/org/wso2/carbon/apimgt/governance/rest/api/dto/DevportalGovernanceKeyManagerScopeDTO.java | DTO for KM scoping on bindings. |
| components/apimgt/org.wso2.carbon.apimgt.governance.impl/src/main/java/org/wso2/carbon/apimgt/governance/impl/util/KeyManagerContextResolver.java | Adds consistent KM context resolution (UUID/name + super-tenant fallback). |
| components/apimgt/org.wso2.carbon.apimgt.governance.impl/src/main/java/org/wso2/carbon/apimgt/governance/impl/RulesetManager.java | Prevents deleting rulesets still bound to templates. |
| components/apimgt/org.wso2.carbon.apimgt.governance.impl/src/main/java/org/wso2/carbon/apimgt/governance/impl/listener/APIMGovServerStartupShutdownListener.java | Adds startup synchronization of server-backed template fields. |
| components/apimgt/org.wso2.carbon.apimgt.governance.impl/src/main/java/org/wso2/carbon/apimgt/governance/impl/DevportalGovernanceValidator.java | Adds synchronous Spectral validator for snapshot-backed app governance. |
| components/apimgt/org.wso2.carbon.apimgt.governance.impl/src/main/java/org/wso2/carbon/apimgt/governance/impl/DevportalGovernanceTemplateConfigSynchronizer.java | Reconciles template formConfig with server-backed groups/attributes at startup. |
| components/apimgt/org.wso2.carbon.apimgt.governance.impl/src/main/java/org/wso2/carbon/apimgt/governance/impl/dao/RulesetMgtDAO.java | Adds DAO contract to list templates associated with a ruleset. |
| components/apimgt/org.wso2.carbon.apimgt.governance.impl/src/main/java/org/wso2/carbon/apimgt/governance/impl/dao/impl/RulesetMgtDAOImpl.java | Implements ruleset→templates association query. |
| components/apimgt/org.wso2.carbon.apimgt.governance.impl/src/main/java/org/wso2/carbon/apimgt/governance/impl/dao/constants/SQLConstants.java | Adds SQL for templates, bindings, snapshots, and association queries. |
| components/apimgt/org.wso2.carbon.apimgt.governance.impl/src/main/java/org/wso2/carbon/apimgt/governance/impl/ArtifactGovernanceFactory.java | Adds APPLICATION artifact type handler wiring. |
| components/apimgt/org.wso2.carbon.apimgt.governance.impl/src/main/java/org/wso2/carbon/apimgt/governance/impl/ApplicationGovernanceHandler.java | Adds artifact governance handler stub for applications (enables future async evaluation). |
| components/apimgt/org.wso2.carbon.apimgt.governance.api/src/main/java/org/wso2/carbon/apimgt/governance/api/model/RuleType.java | Adds APP_INFO / APP_SUBSCRIPTION / APP_OAUTH rule types. |
| components/apimgt/org.wso2.carbon.apimgt.governance.api/src/main/java/org/wso2/carbon/apimgt/governance/api/model/KeyManagerGovernanceContext.java | Adds normalized KM context model for enforcement and rule authoring. |
| components/apimgt/org.wso2.carbon.apimgt.governance.api/src/main/java/org/wso2/carbon/apimgt/governance/api/model/ExtendedArtifactType.java | Adds APPLICATION extended artifact type. |
| components/apimgt/org.wso2.carbon.apimgt.governance.api/src/main/java/org/wso2/carbon/apimgt/governance/api/model/DevportalGovernanceTemplateList.java | Adds template list domain model. |
| components/apimgt/org.wso2.carbon.apimgt.governance.api/src/main/java/org/wso2/carbon/apimgt/governance/api/model/DevportalGovernanceTemplate.java | Adds template domain model. |
| components/apimgt/org.wso2.carbon.apimgt.governance.api/src/main/java/org/wso2/carbon/apimgt/governance/api/model/DevportalGovernanceRulesetSnapshotKeyManagerScope.java | Adds KM scope model for snapshotted rulesets. |
| components/apimgt/org.wso2.carbon.apimgt.governance.api/src/main/java/org/wso2/carbon/apimgt/governance/api/model/DevportalGovernanceRulesetSnapshot.java | Adds ruleset snapshot domain model. |
| components/apimgt/org.wso2.carbon.apimgt.governance.api/src/main/java/org/wso2/carbon/apimgt/governance/api/model/DevportalGovernanceRulesetBinding.java | Adds template↔ruleset binding domain model. |
| components/apimgt/org.wso2.carbon.apimgt.governance.api/src/main/java/org/wso2/carbon/apimgt/governance/api/model/DevportalGovernanceKeyManagerScope.java | Adds KM scope domain model for bindings. |
| components/apimgt/org.wso2.carbon.apimgt.governance.api/src/main/java/org/wso2/carbon/apimgt/governance/api/model/DevportalGovernanceApplicationSnapshot.java | Adds application snapshot domain model. |
| components/apimgt/org.wso2.carbon.apimgt.governance.api/src/main/java/org/wso2/carbon/apimgt/governance/api/model/ArtifactType.java | Adds APPLICATION artifact type. |
| components/apimgt/org.wso2.carbon.apimgt.governance.api/src/main/java/org/wso2/carbon/apimgt/governance/api/model/APIMGovernableState.java | Adds app/sub/key governable states. |
| components/apimgt/org.wso2.carbon.apimgt.governance.api/src/main/java/org/wso2/carbon/apimgt/governance/api/error/APIMGovExceptionCodes.java | Adds exception codes for templates, snapshots, and template/ruleset associations. |
| components/apimgt/org.wso2.carbon.apimgt.governance.api/src/main/java/org/wso2/carbon/apimgt/governance/api/APIMGovernanceAPIConstants.java | Adds template API paths + paginated URL constants. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+372
to
380
| // Hydrate hidden/default fields from the bound governance template before | ||
| // validation, so callers can omit fields the admin has locked or pre-filled. | ||
| TemplateDefaultsApplier.applyTo(body, username, organization, log); | ||
| DevportalGovernanceValidationUtil.validateApplicationCreate(body, organization, log); | ||
| Application createdApplication = preProcessAndAddApplication(username, body, organization, | ||
| orgInfo.getOrganizationId()); | ||
| DevportalGovernanceValidationUtil.captureApplicationSnapshot(createdApplication, body.getTemplateId(), | ||
| organization, log); | ||
| ApplicationDTO createdApplicationDTO = ApplicationMappingUtil.fromApplicationtoDTO(createdApplication); |
Comment on lines
+164
to
+173
| for (RuleViolation v : violations) { | ||
| TemplateDefaultViolationDTO dto = new TemplateDefaultViolationDTO(); | ||
| dto.setRuleName(v.getRuleName()); | ||
| dto.setRulesetId(v.getRulesetId()); | ||
| dto.setViolatedPath(v.getViolatedPath()); | ||
| dto.setMessage(v.getRuleMessage()); | ||
| if (v.getSeverity() != null) { | ||
| dto.setSeverity(TemplateDefaultViolationDTO.SeverityEnum.fromValue(v.getSeverity().toString())); | ||
| } | ||
| result.getViolations().add(dto); |
Comment on lines
+181
to
+195
| int templateCount = templateList.getCount(); | ||
| List<DevportalGovernanceTemplateDTO> paginatedTemplates = new ArrayList<>(); | ||
| DevportalGovernanceTemplateListDTO paginatedTemplateListDTO = new DevportalGovernanceTemplateListDTO(); | ||
| paginatedTemplateListDTO.setCount(Math.min(templateCount, limit)); | ||
|
|
||
| if (offset > templateCount) { | ||
| offset = RestApiConstants.PAGINATION_OFFSET_DEFAULT; | ||
| } | ||
|
|
||
| int start = offset; | ||
| int end = Math.min(templateCount, start + limit); | ||
| List<DevportalGovernanceTemplate> templates = templateList.getTemplateList(); | ||
| for (int i = start; i < end; i++) { | ||
| paginatedTemplates.add(TemplateMappingUtil.fromDevportalGovernanceTemplateToDTO(templates.get(i))); | ||
| } |
Comment on lines
+23
to
28
| import org.wso2.carbon.apimgt.api.APIConsumer; | ||
| import org.wso2.carbon.apimgt.api.APIManagementException; | ||
| import org.wso2.carbon.apimgt.impl.APIConstants; | ||
| import org.wso2.carbon.apimgt.impl.APIConstants.ApplicationAttributes; | ||
| import org.wso2.carbon.apimgt.impl.APIManagerFactory; | ||
| import org.wso2.carbon.apimgt.impl.utils.APIUtil; |
Comment on lines
+92
to
+109
| @Override | ||
| public boolean isArtifactAvailable(String artifactRefId, String organization) throws APIMGovernanceException { | ||
| try { | ||
| Application application = ApiMgtDAO.getInstance().getApplicationByUUID(artifactRefId); | ||
| return application != null; | ||
| } catch (APIManagementException e) { | ||
| throw new APIMGovernanceException(APIMGovExceptionCodes.ERROR_WHILE_CHECKING_APPLICATION_AVAILABILITY, | ||
| e, artifactRefId); | ||
| } | ||
| } | ||
|
|
||
| @Override | ||
| public boolean isArtifactVisibleToUser(String artifactRefId, String username, | ||
| String organization) throws APIMGovernanceException { | ||
| try { | ||
| Application application = ApiMgtDAO.getInstance().getApplicationByUUID(artifactRefId); | ||
| return application != null; | ||
| } catch (APIManagementException e) { |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Note
Due to the large number of review comments, Critical severity comments were prioritized as inline comments.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
components/apimgt/org.wso2.carbon.apimgt.rest.api.admin.v1/src/main/resources/admin-api.yaml (1)
1593-1599:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAdd missing token-type-upgrade scope to
GET /applicationssecurity.
GET /applicationsis missingapim:app_token_type_upgrade, which breaks the expected admin UI authorization path for token-type upgrade flows.🔧 Proposed fix
security: - OAuth2Security: - apim:admin - apim:app_settings_change - apim:app_owner_change + - apim:app_token_type_upgrade - apim:app_import_export - apim:admin_application_viewBased on learnings: "Ensure that in admin-api.yaml files ... the apim:app_token_type_upgrade scope is included in the GET /applications endpoint's security block ... Apply this consistently across all admin-api.yaml resources where GET /applications is exposed."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@components/apimgt/org.wso2.carbon.apimgt.rest.api.admin.v1/src/main/resources/admin-api.yaml` around lines 1593 - 1599, The GET /applications operation's security block (OAuth2Security) is missing the apim:app_token_type_upgrade scope which prevents admin UI token-type-upgrade flows; update the OAuth2Security scopes for the GET /applications entry in admin-api.yaml to include "apim:app_token_type_upgrade" alongside the existing scopes (e.g., apim:admin, apim:app_settings_change, apim:app_owner_change, apim:app_import_export, apim:admin_application_view) so the admin UI can authorize token-type upgrades — apply the same addition wherever GET /applications is defined in admin-api.yaml files.components/apimgt/org.wso2.carbon.apimgt.rest.api.common/src/main/resources/admin-api.yaml (1)
1593-1599:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAdd missing
apim:app_token_type_upgradescope toGET /applications.The security block for
GET /applicationsstill omitsapim:app_token_type_upgrade, which can break admin UI flows that list applications before token-type upgrades.🔧 Proposed spec fix
security: - OAuth2Security: - apim:admin - apim:app_settings_change - apim:app_owner_change + - apim:app_token_type_upgrade - apim:app_import_export - apim:admin_application_viewBased on learnings: Ensure
apim:app_token_type_upgradeis included inGET /applicationssecurity inadmin-api.yamlto support admin UI listing flow.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@components/apimgt/org.wso2.carbon.apimgt.rest.api.common/src/main/resources/admin-api.yaml` around lines 1593 - 1599, The OAuth2Security scopes for the GET /applications operation are missing apim:app_token_type_upgrade; update the security block in admin-api.yaml for the GET /applications operation (the OAuth2Security entry) to include the apim:app_token_type_upgrade scope alongside apim:admin, apim:app_settings_change, apim:app_owner_change, apim:app_import_export, and apim:admin_application_view so the admin UI can list applications prior to token-type upgrades.
🟠 Major comments (23)
features/apimgt/org.wso2.carbon.apimgt.governance.feature/src/main/resources/default-rulesets/wso2_devportal_application_baseline.yaml-33-49 (1)
33-49:⚠️ Potential issue | 🟠 Major | ⚡ Quick winThe
descriptionfield requirement can be silently bypassed when missing.When the
$.application.descriptionJSONPath doesn't exist, Spectral does not apply the rule'sthenclause—it skips execution entirely. This means the "required" validation provides no guarantee and the field can be omitted without raising an error.Fix by targeting the parent object with a schema-based validation:
Proposed fix
'application-description-required': given: - - '$.application.description' + - '$.application' severity: error then: - function: truthy + function: schema + functionOptions: + schema: + type: object + required: + - description + properties: + description: + type: string + minLength: 1🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@features/apimgt/org.wso2.carbon.apimgt.governance.feature/src/main/resources/default-rulesets/wso2_devportal_application_baseline.yaml` around lines 33 - 49, The current rules 'application-description-required' and 'application-description-min-length' target '$.application.description' which Spectral skips when the path is missing; change both to target the parent '$.application' instead and use a field-based/ schema-based then clause (e.g., in 'application-description-required' use then: field: description with function: truthy or a schema that lists description as required, and in 'application-description-min-length' use then: field: description with function: length and functionOptions.min: 10) so the validator runs when the application object exists and will raise an error if description is absent or too short; update rule names/comments accordingly to reference 'application' parent and the 'description' field.components/apimgt/org.wso2.carbon.apimgt.governance.api/src/main/java/org/wso2/carbon/apimgt/governance/api/model/APIMGovernableState.java-33-37 (1)
33-37:⚠️ Potential issue | 🟠 Major | ⚡ Quick winOrdinal-coupled dependency resolution now mixes unrelated governance flows.
Line 60-Line 63 derives dependencies by enum order. After adding
APP_*/SUB_*/KEY_*afterAPI_PUBLISH(Line 33-Line 37), calls likegetDependentGovernableStates(APP_CREATE)will also include API states, which is incorrect for app-governance evaluation.Suggested fix (explicit dependency mapping)
public static List<APIMGovernableState> getDependentGovernableStates(APIMGovernableState state) { - return Stream.of(values()) - .filter(s -> s.ordinal() <= state.ordinal()) - .collect(Collectors.toList()); + switch (state) { + case API_CREATE: + return List.of(API_CREATE); + case API_UPDATE: + return List.of(API_CREATE, API_UPDATE); + case API_DEPLOY: + return List.of(API_CREATE, API_UPDATE, API_DEPLOY); + case API_PUBLISH: + return List.of(API_CREATE, API_UPDATE, API_DEPLOY, API_PUBLISH); + case APP_CREATE: + return List.of(APP_CREATE); + case APP_UPDATE: + return List.of(APP_CREATE, APP_UPDATE); + case SUB_CREATE: + return List.of(SUB_CREATE); + case KEY_GENERATE: + return List.of(KEY_GENERATE); + default: + return List.of(state); + } }Also applies to: 60-63
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@components/apimgt/org.wso2.carbon.apimgt.governance.api/src/main/java/org/wso2/carbon/apimgt/governance/api/model/APIMGovernableState.java` around lines 33 - 37, The enum APIMGovernableState and its dependent-resolution logic rely on enum ordinal/order which now mixes unrelated flows (e.g., APP_CREATE pulls in API_PUBLISH); change getDependentGovernableStates to use an explicit, static Map<APIMGovernableState, Set<APIMGovernableState>> (or EnumMap) that lists dependencies for each enum constant (API_PUBLISH, APP_CREATE, APP_UPDATE, SUB_CREATE, KEY_GENERATE) instead of using ordinal ranges, populate the mapping with the correct dependencies for each state, and update any code that inferred dependencies by order to consult this map so dependency resolution is deterministic and decoupled from enum declaration order.features/apimgt/org.wso2.carbon.apimgt.core.feature/src/main/resources/sql/mysql.sql-2889-2892 (1)
2889-2892:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftPrevent multiple default templates in the same scope.
Line 2890 and Line 2891 can produce multiple default templates for one organization (or multiple global defaults), which makes default resolution ambiguous. Please enforce this invariant at the DB layer (or with strict transactional guardrails in write paths).
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@features/apimgt/org.wso2.carbon.apimgt.core.feature/src/main/resources/sql/mysql.sql` around lines 2889 - 2892, Add a DB-level uniqueness guard so only one template can be marked default per scope: create one or two STORED generated columns that are populated only when IS_DEFAULT=1 (e.g. default_org AS (CASE WHEN IS_DEFAULT=1 THEN ORGANIZATION ELSE NULL END) and default_global AS (CASE WHEN IS_DEFAULT=1 THEN IS_GLOBAL ELSE NULL END)) and then add a unique index on (default_global, default_org) to enforce at most one IS_DEFAULT=1 row per organization/global scope; reference IS_DEFAULT, IS_GLOBAL, ORGANIZATION to locate where to add the generated columns and the unique index.features/apimgt/org.wso2.carbon.apimgt.core.feature/src/main/resources/sql/oracle.sql-4311-4314 (1)
4311-4314:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftEnforce one default template per resolution scope.
Line 4312 and Line 4313 allow multiple “default” rows, which can make local/global default resolution non-deterministic under concurrent updates. Add a DB-level uniqueness guard for local-default-per-organization and global-default singleton behavior.
Possible Oracle-side enforcement
+CREATE UNIQUE INDEX UK_GOV_DPT_DEFAULT_LOCAL +ON GOV_DEVPORTAL_TEMPLATE ( + CASE WHEN IS_DEFAULT = 1 AND IS_GLOBAL = 0 THEN ORGANIZATION END +); + +CREATE UNIQUE INDEX UK_GOV_DPT_DEFAULT_GLOBAL +ON GOV_DEVPORTAL_TEMPLATE ( + CASE WHEN IS_DEFAULT = 1 AND IS_GLOBAL = 1 THEN 'GLOBAL' END +);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@features/apimgt/org.wso2.carbon.apimgt.core.feature/src/main/resources/sql/oracle.sql` around lines 4311 - 4314, The table with columns STATUS, IS_DEFAULT, IS_GLOBAL and ORGANIZATION needs DB-level uniqueness: add a function-based unique index that enforces at most one local default per organization (uniqueness on ORGANIZATION only when IS_DEFAULT=1) and another function-based unique index that enforces a single global default across the whole table (uniqueness on a constant only when IS_GLOBAL=1); create these indexes (e.g., UQ_<table>_LOCAL_DEFAULT and UQ_<table>_GLOBAL_DEFAULT) using Oracle conditional expressions (CASE/DECODE) so rows with IS_DEFAULT=1 map to ORGANIZATION and rows with IS_GLOBAL=1 map to a fixed value, preventing multiple defaults under concurrent updates.components/apimgt/org.wso2.carbon.apimgt.governance.api/src/main/java/org/wso2/carbon/apimgt/governance/api/model/KeyManagerGovernanceContext.java-153-156 (1)
153-156: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick winInconsistent collection immutability pattern.
The setters for
availableGrantTypes,supportedAdditionalPropertyKeys, andrawPropertiesstore mutable collections, while sibling governance models (DevportalGovernanceTemplate,DevportalGovernanceApplicationSnapshot,DevportalGovernanceRulesetBinding) use unmodifiable wrappers. For consistency across the governance API, align with the unmodifiable pattern.♻️ Apply unmodifiable pattern to collection setters
public void setAvailableGrantTypes(List<String> availableGrantTypes) { - this.availableGrantTypes = availableGrantTypes == null - ? Collections.emptyList() : new ArrayList<>(availableGrantTypes); + if (availableGrantTypes == null) { + this.availableGrantTypes = Collections.emptyList(); + } else { + this.availableGrantTypes = Collections.unmodifiableList(new ArrayList<>(availableGrantTypes)); + } }public void setSupportedAdditionalPropertyKeys(List<String> supportedAdditionalPropertyKeys) { - this.supportedAdditionalPropertyKeys = supportedAdditionalPropertyKeys == null - ? Collections.emptyList() : new ArrayList<>(supportedAdditionalPropertyKeys); + if (supportedAdditionalPropertyKeys == null) { + this.supportedAdditionalPropertyKeys = Collections.emptyList(); + } else { + this.supportedAdditionalPropertyKeys = Collections.unmodifiableList(new ArrayList<>(supportedAdditionalPropertyKeys)); + } }public void setRawProperties(Map<String, Object> rawProperties) { - this.rawProperties = rawProperties == null ? new LinkedHashMap<>() : new LinkedHashMap<>(rawProperties); + if (rawProperties == null) { + this.rawProperties = Collections.emptyMap(); + } else { + this.rawProperties = Collections.unmodifiableMap(new LinkedHashMap<>(rawProperties)); + } }Also applies to: 258-261, 267-269
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@components/apimgt/org.wso2.carbon.apimgt.governance.api/src/main/java/org/wso2/carbon/apimgt/governance/api/model/KeyManagerGovernanceContext.java` around lines 153 - 156, The setters in KeyManagerGovernanceContext (notably setAvailableGrantTypes, setSupportedAdditionalPropertyKeys, and setRawProperties) currently assign mutable copies; change them to wrap the assigned collections with Collections.unmodifiableList / Collections.unmodifiableSet / Collections.unmodifiableMap as appropriate (or Collections.unmodifiableList for lists and Collections.unmodifiableMap for maps) and ensure null still results in Collections.emptyList()/emptyMap(); update setAvailableGrantTypes, setSupportedAdditionalPropertyKeys, and setRawProperties to store unmodifiable collections to match the pattern used in DevportalGovernanceTemplate, DevportalGovernanceApplicationSnapshot, and DevportalGovernanceRulesetBinding.components/apimgt/org.wso2.carbon.apimgt.governance.api/src/main/java/org/wso2/carbon/apimgt/governance/api/model/DevportalGovernanceTemplate.java-87-90 (1)
87-90: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick winInconsistent immutability pattern for
tagsfield.The
setTagsmethod stores a mutableArrayList, whilesetFormConfig(lines 110-113) andsetRulesetBindings(lines 213-217) store unmodifiable collections. For consistency and clarity, apply the same unmodifiable pattern here.♻️ Align tags with the unmodifiable pattern
public void setTags(List<String> tags) { - this.tags = tags == null ? new ArrayList<>() : new ArrayList<>(tags); + if (tags == null) { + this.tags = Collections.emptyList(); + } else { + this.tags = Collections.unmodifiableList(new ArrayList<>(tags)); + } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@components/apimgt/org.wso2.carbon.apimgt.governance.api/src/main/java/org/wso2/carbon/apimgt/governance/api/model/DevportalGovernanceTemplate.java` around lines 87 - 90, The setTags method in DevportalGovernanceTemplate currently assigns a mutable ArrayList to the tags field; change it to follow the same unmodifiable pattern used by setFormConfig and setRulesetBindings by wrapping a defensive copy in an unmodifiable list (use Collections.emptyList() when tags is null, otherwise Collections.unmodifiableList(new ArrayList<>(tags))) so tags is stored immutably.components/apimgt/org.wso2.carbon.apimgt.governance.impl/src/main/java/org/wso2/carbon/apimgt/governance/impl/ApplicationGovernanceHandler.java-103-112 (1)
103-112:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy lift
isArtifactVisibleToUsercurrently bypasses per-user visibility.
usernameis never used, and the method returnstruefor any existing application. That turns a visibility check into a plain existence check, which will misreport access for applications outside the caller's ownership or sharing scope.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@components/apimgt/org.wso2.carbon.apimgt.governance.impl/src/main/java/org/wso2/carbon/apimgt/governance/impl/ApplicationGovernanceHandler.java` around lines 103 - 112, The method isArtifactVisibleToUser currently ignores username and only checks existence via ApiMgtDAO.getInstance().getApplicationByUUID(artifactRefId); update it to enforce per-user visibility: fetch the Application, then verify ownership and/or sharing before returning true (e.g., check application.getOwner().equals(username) and any shared/tenant access lists on the Application), returning false when the user is not owner and not in shared-access; preserve the APIManagementException handling and throw the same APIMGovernanceException when DAO calls fail.components/apimgt/org.wso2.carbon.apimgt.governance.impl/src/main/java/org/wso2/carbon/apimgt/governance/impl/ApplicationGovernanceHandler.java-93-112 (1)
93-112:⚠️ Potential issue | 🟠 Major | ⚡ Quick winScope application lookups to the requested organization.
Every lookup here is keyed only by UUID, so an application from another organization will still be treated as available/visible and its metadata can be returned from
getName,getOwner, andgetArtifactProject. Please enforceorganizationduring lookup or reject mismatchedapplication.getOrganization()values before returning data.Also applies to: 128-172, 192-200
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@components/apimgt/org.wso2.carbon.apimgt.governance.impl/src/main/java/org/wso2/carbon/apimgt/governance/impl/ApplicationGovernanceHandler.java` around lines 93 - 112, The current UUID-only lookups (in isArtifactAvailable and isArtifactVisibleToUser and the other mentioned methods like getName, getOwner, getArtifactProject) can return applications from other organizations; after retrieving Application application = ApiMgtDAO.getInstance().getApplicationByUUID(artifactRefId) you must enforce the requested organization by either querying with organization-aware DAO call or validating application.getOrganization() equals the incoming organization and treat mismatches as "not found"/not visible (throw APIMGovernanceException or return false as appropriate). Update all affected methods (isArtifactAvailable, isArtifactVisibleToUser, getName, getOwner, getArtifactProject) to perform this check and use the APIMGovExceptionCodes or boolean return semantics already used in each method when rejecting mismatched organizations.features/apimgt/org.wso2.carbon.apimgt.core.feature/src/main/resources/sql/mssql.sql-3130-3131 (1)
3130-3131:⚠️ Potential issue | 🟠 Major | ⚡ Quick winUse Unicode column types for persisted form/ruleset payloads.
FORM_CONFIGandYAML_CONTENTcan carry user-authored labels, descriptions, and rule text. On SQL Server,VARCHAR(MAX)is code-page based, so non-ASCII content will be lossy; these payload columns should beNVARCHAR(MAX).Example MSSQL fix
- FORM_CONFIG VARCHAR(MAX) NOT NULL, + FORM_CONFIG NVARCHAR(MAX) NOT NULL, ... - FORM_CONFIG VARCHAR(MAX) NOT NULL, + FORM_CONFIG NVARCHAR(MAX) NOT NULL, ... - YAML_CONTENT VARCHAR(MAX) NOT NULL, + YAML_CONTENT NVARCHAR(MAX) NOT NULL,Also applies to: 3176-3177, 3195-3196
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@features/apimgt/org.wso2.carbon.apimgt.core.feature/src/main/resources/sql/mssql.sql` around lines 3130 - 3131, The FORM_CONFIG and YAML_CONTENT columns are defined as VARCHAR(MAX) which is non-Unicode and will corrupt non-ASCII user-provided content; update their column types to NVARCHAR(MAX) wherever they are declared (e.g., the FORM_CONFIG and YAML_CONTENT column definitions and their corresponding schema blocks), leaving the hash columns (FORM_CONFIG_HASH/YAML_CONTENT_HASH) unchanged unless you need Unicode there too; ensure all occurrences in the MSSQL table creation/alter statements are changed to NVARCHAR(MAX).features/apimgt/org.wso2.carbon.apimgt.core.feature/src/main/resources/sql/mssql.sql-3123-3142 (1)
3123-3142:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAdd database-level uniqueness constraints for default templates across all database dialects.
The
IS_DEFAULT/IS_GLOBALinvariant is only coordinated in application code. Even with transaction boundaries, concurrent create/update flows can race and leave multiple default templates for the same organization or globally, making resolution nondeterministic. Database-level constraints are required.For MSSQL, PostgreSQL, Oracle, MySQL, and H2 schema files, add filtered/conditional unique indexes or constraints to enforce:
- At most one row per organization with
IS_DEFAULT = 1 AND IS_GLOBAL = 0- At most one row globally with
IS_DEFAULT = 1 AND IS_GLOBAL = 1Example for MSSQL:
CREATE UNIQUE INDEX UK_GOV_DPT_DEFAULT_LOCAL ON GOV_DEVPORTAL_TEMPLATE (ORGANIZATION) WHERE IS_DEFAULT = 1 AND IS_GLOBAL = 0; CREATE UNIQUE INDEX UK_GOV_DPT_DEFAULT_GLOBAL ON GOV_DEVPORTAL_TEMPLATE (IS_GLOBAL) WHERE IS_DEFAULT = 1 AND IS_GLOBAL = 1;Equivalent constraints needed in mysql.sql, postgresql.sql, oracle.sql, h2.sql, and other supported database schemas.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@features/apimgt/org.wso2.carbon.apimgt.core.feature/src/main/resources/sql/mssql.sql` around lines 3123 - 3142, Add DB-level uniqueness to prevent multiple default templates by creating conditional/filtered unique indexes on the GOV_DEVPORTAL_TEMPLATE table: add a unique filtered index UK_GOV_DPT_DEFAULT_LOCAL on (ORGANIZATION) where IS_DEFAULT = 1 AND IS_GLOBAL = 0, and a unique filtered index UK_GOV_DPT_DEFAULT_GLOBAL on (IS_GLOBAL) where IS_DEFAULT = 1 AND IS_GLOBAL = 1; implement equivalent constructs in the other schema files (mysql.sql, postgresql.sql, oracle.sql, h2.sql) using each dialect's syntax for partial/functional/filtered unique indexes or constraints so the invariants around IS_DEFAULT and IS_GLOBAL are enforced at the database level for the GOV_DEVPORTAL_TEMPLATE table.components/apimgt/org.wso2.carbon.apimgt.governance.impl/src/main/java/org/wso2/carbon/apimgt/governance/impl/listener/APIMGovServerStartupShutdownListener.java-69-72 (1)
69-72:⚠️ Potential issue | 🟠 Major | ⚡ Quick winGuard JMS subscription to avoid startup-time NPE when Event Hub receiver config is absent.
Line 70 dereferences
jmsTransportHandlerForEventHubeven though constructor initialization is conditional. Add a null check before subscribing.💡 Suggested fix
APIMGovernanceUtil.loadDefaultRulesets(MultitenantConstants.SUPER_TENANT_DOMAIN_NAME); APIMGovernanceUtil.loadDefaultPolicies(MultitenantConstants.SUPER_TENANT_DOMAIN_NAME); DevportalGovernanceTemplateConfigSynchronizer.synchronizeAllTemplates(); - jmsTransportHandlerForEventHub - .subscribeForJmsEvents(APIConstants.TopicNames.TOPIC_NOTIFICATION, - new APIMGovernanceMessageListener()); + if (jmsTransportHandlerForEventHub != null) { + jmsTransportHandlerForEventHub.subscribeForJmsEvents( + APIConstants.TopicNames.TOPIC_NOTIFICATION, + new APIMGovernanceMessageListener()); + } else { + log.warn("Event Hub JMS receiver configuration is not available; skipping JMS subscription."); + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@components/apimgt/org.wso2.carbon.apimgt.governance.impl/src/main/java/org/wso2/carbon/apimgt/governance/impl/listener/APIMGovServerStartupShutdownListener.java` around lines 69 - 72, The call to jmsTransportHandlerForEventHub.subscribeForJmsEvents(...) in APIMGovServerStartupShutdownListener can NPE because jmsTransportHandlerForEventHub is only conditionally initialized in the constructor; add a null guard before calling subscribeForJmsEvents (e.g., if (jmsTransportHandlerForEventHub != null) { jmsTransportHandlerForEventHub.subscribeForJmsEvents(APIConstants.TopicNames.TOPIC_NOTIFICATION, new APIMGovernanceMessageListener()); } ), and optionally log a debug/info message when the handler is absent so startup behavior is clear.components/apimgt/org.wso2.carbon.apimgt.rest.api.admin.v1/src/main/java/org/wso2/carbon/apimgt/rest/api/admin/v1/impl/SettingsApiServiceImpl.java-65-85 (1)
65-85:⚠️ Potential issue | 🟠 Major | ⚡ Quick winKeep
/settingsresilient to malformed attribute payloads.The fallback path currently catches only
APIManagementException. Mapping errors (ClassCastException,NullPointerException) will still fail the endpoint instead of returning partial settings.🔧 Proposed fix
- } catch (APIManagementException e) { + } catch (APIManagementException | RuntimeException e) { if (log.isDebugEnabled()) { log.debug("Failed to load application attributes for settings response", e); } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@components/apimgt/org.wso2.carbon.apimgt.rest.api.admin.v1/src/main/java/org/wso2/carbon/apimgt/rest/api/admin/v1/impl/SettingsApiServiceImpl.java` around lines 65 - 85, The try-block that loads application attributes via APIConsumer.getAppAttributesFromConfig and maps JSONArray items into attrs currently only catches APIManagementException, so mapping errors (e.g., ClassCastException, NullPointerException) can break the /settings response; update the error handling around the APIConsumer/getAppAttributesFromConfig and the loop that builds attr maps (references: APIConsumer, getAppAttributesFromConfig, attrArray, settingsDTO, APIConstants.ApplicationAttributes) to also catch mapping/runtime exceptions (at minimum ClassCastException and NullPointerException, or a broader Exception) and handle them by logging the error (debug or warn) and continuing so partial attributes are returned and settingsDTO.setApplicationAttributes(attrs) still executes.components/apimgt/org.wso2.carbon.apimgt.rest.api.admin.v1/src/main/java/org/wso2/carbon/apimgt/rest/api/admin/v1/impl/SettingsApiServiceImpl.java-75-77 (1)
75-77:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDo not coerce config booleans/nulls into strings.
String.valueOf(...)turns booleans into"true"/"false"and nulls into"null", which can break consumers ofapplicationAttributesand changes data semantics.🔧 Proposed fix
- attr.put("required", String.valueOf(obj.get(APIConstants.ApplicationAttributes.REQUIRED))); - attr.put("hidden", String.valueOf(obj.get(APIConstants.ApplicationAttributes.HIDDEN))); - attr.put("type", String.valueOf(obj.get(APIConstants.ApplicationAttributes.TYPE))); + attr.put("required", obj.get(APIConstants.ApplicationAttributes.REQUIRED)); + attr.put("hidden", obj.get(APIConstants.ApplicationAttributes.HIDDEN)); + Object type = obj.get(APIConstants.ApplicationAttributes.TYPE); + if (type != null) { + attr.put("type", type.toString()); + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@components/apimgt/org.wso2.carbon.apimgt.rest.api.admin.v1/src/main/java/org/wso2/carbon/apimgt/rest/api/admin/v1/impl/SettingsApiServiceImpl.java` around lines 75 - 77, The code in SettingsApiServiceImpl is coercing boolean/null values to strings by using String.valueOf(...) for keys APIConstants.ApplicationAttributes.REQUIRED, HIDDEN and TYPE; instead, stop stringifying and put the raw values into the attr map (e.g., attr.put("required", obj.get(APIConstants.ApplicationAttributes.REQUIRED))) or, if the map must not contain nulls, explicitly handle nulls by omitting the entry or inserting a JSON-aware null (e.g., JSONObject.NULL) so the original boolean/null semantics are preserved for applicationAttributes.components/apimgt/org.wso2.carbon.apimgt.governance.rest.api/src/main/java/org/wso2/carbon/apimgt/governance/rest/api/mappings/TemplateMappingUtil.java-111-120 (1)
111-120:⚠️ Potential issue | 🟠 Major | ⚡ Quick winGuard against null elements in list mappings.
These loops assume non-null list items; a null entry can cause
NullPointerExceptionand return a 500 for malformed input instead of graceful handling.🔧 Proposed fix
for (int i = 0; i < bindingDTOs.size(); i++) { DevportalGovernanceRulesetBindingDTO bindingDTO = bindingDTOs.get(i); + if (bindingDTO == null) { + continue; + } DevportalGovernanceRulesetBinding binding = new DevportalGovernanceRulesetBinding(); ... } for (DevportalGovernanceRulesetBinding binding : bindings) { + if (binding == null) { + continue; + } DevportalGovernanceRulesetBindingDTO bindingDTO = new DevportalGovernanceRulesetBindingDTO(); ... } for (DevportalGovernanceKeyManagerScopeDTO keyManagerScopeDTO : keyManagerScopeDTOs) { + if (keyManagerScopeDTO == null) { + continue; + } DevportalGovernanceKeyManagerScope keyManagerScope = new DevportalGovernanceKeyManagerScope(); ... } for (DevportalGovernanceKeyManagerScope keyManagerScope : keyManagerScopes) { + if (keyManagerScope == null) { + continue; + } DevportalGovernanceKeyManagerScopeDTO keyManagerScopeDTO = new DevportalGovernanceKeyManagerScopeDTO(); ... }Also applies to: 132-143, 155-160, 171-176
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@components/apimgt/org.wso2.carbon.apimgt.governance.rest.api/src/main/java/org/wso2/carbon/apimgt/governance/rest/api/mappings/TemplateMappingUtil.java` around lines 111 - 120, The loops in TemplateMappingUtil that iterate over bindingDTOs (and similar loops at the other ranges) assume each list element is non-null and will NPE on malformed input; update the mapping loops (the one using bindingDTOs -> DevportalGovernanceRulesetBinding and the other analogous loops at 132-143, 155-160, 171-176) to first check for null on each DTO element and skip or continue if null before accessing its getters (preserve existing behavior for non-null items), so the method safely ignores null entries instead of throwing a NullPointerException.components/apimgt/org.wso2.carbon.apimgt.governance.rest.api/src/main/java/org/wso2/carbon/apimgt/governance/rest/api/mappings/TemplateMappingUtil.java-55-56 (1)
55-56:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAvoid immutable default for
formConfig.Using
Collections.emptyMap()can fail later withUnsupportedOperationExceptionif any downstream path enrichesformConfig. Use a mutable empty map instead.🔧 Proposed fix
- template.setFormConfig(templateDTO.getFormConfig() == null ? Collections.emptyMap() : - templateDTO.getFormConfig()); + template.setFormConfig(templateDTO.getFormConfig() == null ? new java.util.HashMap<>() : + templateDTO.getFormConfig());🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@components/apimgt/org.wso2.carbon.apimgt.governance.rest.api/src/main/java/org/wso2/carbon/apimgt/governance/rest/api/mappings/TemplateMappingUtil.java` around lines 55 - 56, The code sets template.formConfig to an immutable empty map when templateDTO.getFormConfig() is null by using Collections.emptyMap(), which can cause UnsupportedOperationException if later modified; update TemplateMappingUtil to set a mutable map instead (e.g., new HashMap<>()) when templateDTO.getFormConfig() returns null so template.setFormConfig(...) receives a modifiable Map; locate the assignment around template.setFormConfig and replace the Collections.emptyMap() default with a mutable map constructed from an empty Map or from templateDTO.getFormConfig() as appropriate.components/apimgt/org.wso2.carbon.apimgt.rest.api.store.v1/src/main/java/org/wso2/carbon/apimgt/rest/api/store/v1/impl/ApplicationsApiServiceImpl.java-1349-1354 (1)
1349-1354:⚠️ Potential issue | 🟠 Major | ⚡ Quick winNPE risk:
applicationis dereferenced without a null check.
apiConsumer.getLightweightApplicationByUUID(applicationId)can returnnullfor an unknown/invalid application UUID. The new governance call then evaluatesapplication.getOrganization()(and the subsequentapplication.getId()on Line 1354), which will throwNullPointerExceptionand surface as a 500 instead of a clean 404. Other governed surfaces in this file (e.g., the OAuth delete handler around Line 1846) guard withhandleResourceNotFoundErrorbefore touchingapplication; this legacykeyTypecleanup path should do the same.🛡️ Proposed null/access guard before the governance call
APIConsumer apiConsumer = APIManagerFactory.getInstance().getAPIConsumer(username); Application application = apiConsumer.getLightweightApplicationByUUID(applicationId); + if (application == null) { + RestApiUtil.handleResourceNotFoundError(RestApiConstants.RESOURCE_APPLICATION, applicationId, log); + return null; + } + if (!(orgWideAppUpdateEnabled || RestAPIStoreUtils.isUserOwnerOfApplication(application))) { + RestApiUtil.handleAuthorizationFailure(RestApiConstants.RESOURCE_APPLICATION, applicationId, log); + return null; + } // Devportal Governance gate — legacy keyType surface always targets Resident KM. DevportalGovernanceValidationUtil.validateOAuthAppRemoval(application, null, APIConstants.KeyManager.DEFAULT_KEY_MANAGER, KeyManagerGovernanceContext.Action.OAUTH_APP_CLEANUP, application.getOrganization(), log);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@components/apimgt/org.wso2.carbon.apimgt.rest.api.store.v1/src/main/java/org/wso2/carbon/apimgt/rest/api/store/v1/impl/ApplicationsApiServiceImpl.java` around lines 1349 - 1354, The code dereferences application returned by apiConsumer.getLightweightApplicationByUUID(applicationId) without a null check, risk NPE; fix by adding a guard that calls handleResourceNotFoundError when application is null (same pattern used elsewhere) before invoking DevportalGovernanceValidationUtil.validateOAuthAppRemoval(...) and apiConsumer.cleanUpApplicationRegistrationByApplicationId(...), so both validateOAuthAppRemoval and cleanUpApplicationRegistrationByApplicationId only run when application != null.components/apimgt/org.wso2.carbon.apimgt.rest.api.store.v1/src/main/java/org/wso2/carbon/apimgt/rest/api/store/v1/impl/ApplicationsApiServiceImpl.java-1821-1830 (1)
1821-1830:⚠️ Potential issue | 🟠 Major | ⚡ Quick winSame NPE risk in the OAuth key-mapping cleanup path.
Identical pattern to the legacy
keyTypecleanup:applicationfromgetLightweightApplicationByUUIDis not null-checked beforeapplication.getOrganization()(Line 1829) andapplication.getId()(Line 1830). For consistency withapplicationsApplicationIdOauthKeysKeyMappingIdDelete(Line 1846), add the resource-not-found and owner checks before invoking governance.🛡️ Proposed guard
APIConsumer apiConsumer = APIManagerFactory.getInstance().getAPIConsumer(username); Application application = apiConsumer.getLightweightApplicationByUUID(applicationId); + if (application == null) { + RestApiUtil.handleResourceNotFoundError(RestApiConstants.RESOURCE_APPLICATION, applicationId, log); + return null; + } + if (!RestAPIStoreUtils.isUserOwnerOfApplication(application)) { + RestApiUtil.handleAuthorizationFailure(RestApiConstants.RESOURCE_APPLICATION, applicationId, log); + return null; + } // Devportal Governance gate — runs before the KM-side cleanup call. Resolved KM // is derived from the existing key registration, falling back to Resident. ApplicationKeyDTO existingKey = getApplicationKeyByAppIDAndKeyMapping(applicationId, keyMappingId);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@components/apimgt/org.wso2.carbon.apimgt.rest.api.store.v1/src/main/java/org/wso2/carbon/apimgt/rest/api/store/v1/impl/ApplicationsApiServiceImpl.java` around lines 1821 - 1830, The code calls getLightweightApplicationByUUID(...) and then uses application.getOrganization() and application.getId() without null checks; update ApplicationsApiServiceImpl to mirror the guard used in applicationsApplicationIdOauthKeysKeyMappingIdDelete: after retrieving Application via getLightweightApplicationByUUID(applicationId) verify the application is non-null and the caller is the owner (or throw a ResourceNotFoundException / appropriate 404/403) before calling DevportalGovernanceValidationUtil.validateOAuthAppRemoval(...) and apiConsumer.cleanUpApplicationRegistrationByApplicationIdAndKeyMappingId(...); ensure you reference the same resource-not-found/owner-check logic flow used in applicationsApplicationIdOauthKeysKeyMappingIdDelete so nulls and unauthorized access are handled consistently.components/apimgt/org.wso2.carbon.apimgt.governance.rest.api/src/main/java/org/wso2/carbon/apimgt/governance/rest/api/impl/TemplatesApiServiceImpl.java-141-150 (1)
141-150:⚠️ Potential issue | 🟠 Major | ⚡ Quick winFix pagination bounds before slicing the list.
Negative
offset/limitvalues can still reachtemplates.get(i),offset > totalcurrently jumps back to page 0, andcountis derived frommin(total, limit)instead of the actual slice size. That can return the wrong page and broken pagination metadata to callers.Suggested change
limit = limit != null ? limit : RestApiConstants.PAGINATION_LIMIT_DEFAULT; offset = offset != null ? offset : RestApiConstants.PAGINATION_OFFSET_DEFAULT; + if (limit < 0 || offset < 0) { + throw new APIMGovernanceException(APIMGovExceptionCodes.BAD_REQUEST, + "limit and offset must be non-negative"); + } @@ - paginatedTemplateListDTO.setCount(Math.min(templateCount, limit)); - - if (offset > templateCount) { - offset = RestApiConstants.PAGINATION_OFFSET_DEFAULT; - } - - int start = offset; + int start = Math.min(offset, templateCount); int end = Math.min(templateCount, start + limit); + paginatedTemplateListDTO.setCount(end - start); List<DevportalGovernanceTemplate> templates = templateList.getTemplateList();Also applies to: 181-202
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@components/apimgt/org.wso2.carbon.apimgt.governance.rest.api/src/main/java/org/wso2/carbon/apimgt/governance/rest/api/impl/TemplatesApiServiceImpl.java` around lines 141 - 150, The pagination logic in TemplatesApiServiceImpl (getPaginatedTemplates and its use around the DevportalGovernanceTemplateList/DevportalGovernanceTemplateListDTO flow) must validate and clamp bounds before slicing: ensure limit and offset are non-negative (set to defaults if invalid), if offset >= total return an empty DTO page rather than wrapping to page 0, compute startIndex = min(max(offset,0), total) and endIndex = min(startIndex + max(limit,0), total), slice templates from startIndex to endIndex, and set count to the actual slice size (endIndex - startIndex) and total to the full list size; apply the same corrections to the other occurrence noted (the block at the later template pagination).components/apimgt/org.wso2.carbon.apimgt.governance.impl/src/main/java/org/wso2/carbon/apimgt/governance/impl/DevportalGovernanceManager.java-256-285 (1)
256-285:⚠️ Potential issue | 🟠 Major | ⚡ Quick winTreat omitted binding arrays as empty lists.
rulesetBindingsandkeyManagerScopesare optional in the API model, but both are dereferenced here without null normalization. A request that omits either array will 500 instead of behaving like “no bindings/scopes”.Suggested change
private void prepareRulesetBindings(DevportalGovernanceTemplate template, String organization) throws APIMGovernanceException { + List<DevportalGovernanceRulesetBinding> bindings = template.getRulesetBindings(); + if (bindings == null) { + template.setRulesetBindings(Collections.emptyList()); + return; + } + List<DevportalGovernanceRulesetBinding> preparedBindings = new ArrayList<>(); Set<String> rulesetIds = new HashSet<>(); Set<Integer> bindingOrders = new HashSet<>(); List<String> allowedKeyManagerNames = resolveAllowedKeyManagerNames(template.getFormConfig()); int bindingOrder = 0; - for (DevportalGovernanceRulesetBinding binding : template.getRulesetBindings()) { + for (DevportalGovernanceRulesetBinding binding : bindings) { @@ List<DevportalGovernanceKeyManagerScope> keyManagerScopes = binding.getKeyManagerScopes(); + if (keyManagerScopes == null) { + keyManagerScopes = Collections.emptyList(); + }Also applies to: 345-352
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@components/apimgt/org.wso2.carbon.apimgt.governance.impl/src/main/java/org/wso2/carbon/apimgt/governance/impl/DevportalGovernanceManager.java` around lines 256 - 285, The code dereferences optional arrays without null checks causing NPEs; update DevportalGovernanceManager to normalize null collections to empty lists before iterating or passing to helpers: ensure template.getRulesetBindings() is treated as Collections.emptyList() when null (so the for-loop over template.getRulesetBindings() is skipped safely) and ensure binding.getKeyManagerScopes() is normalized to an empty list (or have prepareKeyManagerScopes accept null and treat it as empty) before calling prepareKeyManagerScopes(binding, ...); apply the same null-to-empty normalization in the other similar block referenced around prepareKeyManagerScopes (lines ~345-352).components/apimgt/org.wso2.carbon.apimgt.governance.impl/src/main/java/org/wso2/carbon/apimgt/governance/impl/DevportalGovernanceManager.java-593-619 (1)
593-619:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftAlign legacy
keyGenerationdry-run coverage with runtime defaulting.Runtime defaulting still applies legacy
formConfig.keyGenerationdefaults to any requested Key Manager, but this dry-run only validates that same legacy section against"Resident Key Manager". A legacy template can therefore publish cleanly and still fail later for a non-resident KM request. Either validate legacy defaults against every KM that can consume them, or narrow the runtime fallback to Resident-only as well.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@components/apimgt/org.wso2.carbon.apimgt.governance.impl/src/main/java/org/wso2/carbon/apimgt/governance/impl/DevportalGovernanceManager.java` around lines 593 - 619, The dry-run only attaches legacy formConfig.keyGeneration to "Resident Key Manager", which is inconsistent with runtime defaulting; in governedKeyManagerSections(Map<String, Object> formConfig) change the legacy handling so that when a legacy map exists and a keyManagersSection is present you iterate the keyManagersSection keys and add the legacy map as the defaults for each KM (e.g., for each entry in keyManagersSection put(entry.getKey(), legacy) when that KM has no per-KM defaults), otherwise keep the current fallback of mapping legacy to "Resident Key Manager"; update references to kmConfig/result merging in governedKeyManagerSections accordingly.components/apimgt/org.wso2.carbon.apimgt.governance.impl/src/main/java/org/wso2/carbon/apimgt/governance/impl/DevportalGovernanceManager.java-258-287 (1)
258-287:⚠️ Potential issue | 🟠 Major | ⚡ Quick winImplicit binding orders can collide with explicit ones.
Negative/omitted orders are replaced with the loop counter, so a payload like
[bindingOrder: 1, omitted]resolves to[1, 1]and is rejected even though only one order was explicitly set. Please assign the next unused order instead of the raw iteration index.Suggested change
- int bindingOrder = 0; + int nextBindingOrder = 0; for (DevportalGovernanceRulesetBinding binding : template.getRulesetBindings()) { @@ - binding.setBindingOrder(binding.getBindingOrder() >= 0 ? binding.getBindingOrder() : bindingOrder); + int resolvedBindingOrder = binding.getBindingOrder(); + if (resolvedBindingOrder < 0) { + while (bindingOrders.contains(nextBindingOrder)) { + nextBindingOrder++; + } + resolvedBindingOrder = nextBindingOrder++; + } + binding.setBindingOrder(resolvedBindingOrder); if (!bindingOrders.add(binding.getBindingOrder())) { throw new APIMGovernanceException(APIMGovExceptionCodes.BAD_REQUEST, "Duplicate binding orders are not allowed for a template"); } @@ - bindingOrder++; + nextBindingOrder = Math.max(nextBindingOrder, resolvedBindingOrder + 1); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@components/apimgt/org.wso2.carbon.apimgt.governance.impl/src/main/java/org/wso2/carbon/apimgt/governance/impl/DevportalGovernanceManager.java` around lines 258 - 287, The current loop in DevportalGovernanceManager uses the loop index bindingOrder as a fallback for negative/omitted DevportalGovernanceRulesetBinding.getBindingOrder(), which can collide with explicit orders; change the fallback logic so that when binding.getBindingOrder() is negative/omitted you compute and assign the next unused non-negative order (e.g., find the smallest non-negative integer not present in the bindingOrders Set) before adding to bindingOrders and preparedBindings; keep the duplicate-order check using bindingOrders as-is and ensure bindingOrder (the loop counter) is only used to advance iteration, not as the assigned order.components/apimgt/org.wso2.carbon.apimgt.governance.impl/src/main/java/org/wso2/carbon/apimgt/governance/impl/DevportalGovernanceTemplateConfigSynchronizer.java-69-91 (1)
69-91:⚠️ Potential issue | 🟠 Major | ⚡ Quick winKeep syncing after one template fails.
The
try/catchwraps the whole batch, so one failing lookup/update stops reconciliation for every remaining template on startup. That leaves later templates with stale server-backed flags until the next restart.Suggested change
- try { - List<DevportalGovernanceTemplate> templates = dao.getAllTemplates(); - Map<String, JSONArray> attributesByOrganization = new HashMap<>(); - for (DevportalGovernanceTemplate template : templates) { - JSONArray applicationAttributes = attributesByOrganization.computeIfAbsent( - template.getOrganization(), - DevportalGovernanceTemplateConfigSynchronizer::getApplicationAttributes); - Map<String, Object> currentFormConfig = template.getFormConfig(); - Map<String, Object> reconciledFormConfig = reconcileFormConfig(currentFormConfig, - applicationSharingEnabled, applicationAttributes); - if (!reconciledFormConfig.equals(currentFormConfig)) { - dao.updateTemplateFormConfig(template.getId(), reconciledFormConfig, template.getOrganization(), - SYSTEM_USER); - updatedCount++; - } - } + try { + List<DevportalGovernanceTemplate> templates = dao.getAllTemplates(); + Map<String, JSONArray> attributesByOrganization = new HashMap<>(); + for (DevportalGovernanceTemplate template : templates) { + try { + JSONArray applicationAttributes = attributesByOrganization.computeIfAbsent( + template.getOrganization(), + DevportalGovernanceTemplateConfigSynchronizer::getApplicationAttributes); + Map<String, Object> currentFormConfig = template.getFormConfig(); + Map<String, Object> reconciledFormConfig = reconcileFormConfig(currentFormConfig, + applicationSharingEnabled, applicationAttributes); + if (!reconciledFormConfig.equals(currentFormConfig)) { + dao.updateTemplateFormConfig(template.getId(), reconciledFormConfig, + template.getOrganization(), SYSTEM_USER); + updatedCount++; + } + } catch (APIMGovernanceException e) { + log.warn("Error while synchronizing Devportal Governance template " + template.getId(), e); + } + } if (log.isDebugEnabled()) { log.debug("Synchronized Devportal Governance template server-backed fields. Updated templates: " + updatedCount); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@components/apimgt/org.wso2.carbon.apimgt.governance.impl/src/main/java/org/wso2/carbon/apimgt/governance/impl/DevportalGovernanceTemplateConfigSynchronizer.java` around lines 69 - 91, The current try/catch around the entire loop stops processing all templates when one template throws; change to catch exceptions per-template so the loop continues: keep dao.getAllTemplates() outside, then for each DevportalGovernanceTemplate in the for-loop wrap the per-template work (calling getApplicationAttributes, reconcileFormConfig, and dao.updateTemplateFormConfig) in its own try/catch, log a warning with template.getId() and template.getOrganization() on failure, and continue to the next template; ensure updatedCount is only incremented on successful update and preserve the existing debug log after the loop in DevportalGovernanceTemplateConfigSynchronizer.components/apimgt/org.wso2.carbon.apimgt.rest.api.store.v1/src/main/java/org/wso2/carbon/apimgt/rest/api/store/v1/utils/DevportalGovernanceValidationUtil.java-366-372 (1)
366-372:⚠️ Potential issue | 🟠 Major | ⚡ Quick winSecret rotation is gated too loosely.
This currently blocks only when both token generation and OAuth app creation are disabled, but the method contract says rotation should be rejected once token generation is disabled. As written, a KM that disables token generation can still pass validation here and fail later downstream.
🐛 Suggested fix
kmContext -> { - if (!kmContext.isEnableTokenGeneration() && !kmContext.isEnableOAuthAppCreation()) { + if (!kmContext.isEnableTokenGeneration()) { return "Key Manager '" + kmContext.getDisplayName() + "' does not allow consumer secret rotation."; } return null; });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@components/apimgt/org.wso2.carbon.apimgt.rest.api.store.v1/src/main/java/org/wso2/carbon/apimgt/rest/api/store/v1/utils/DevportalGovernanceValidationUtil.java` around lines 366 - 372, The validation currently allows secret rotation when token generation is disabled but OAuth app creation is enabled; update the check in DevportalGovernanceValidationUtil (the kmContext lambda used for key manager validation) to reject rotation whenever kmContext.isEnableTokenGeneration() is false (i.e., if token generation is disabled return the "Key Manager '<name>' does not allow consumer secret rotation." message), rather than only when both isEnableTokenGeneration() and isEnableOAuthAppCreation() are false.
🟡 Minor comments (3)
components/apimgt/org.wso2.carbon.apimgt.rest.api.store.v1/src/main/java/org/wso2/carbon/apimgt/rest/api/store/v1/impl/ApplicationsApiServiceImpl.java-376-384 (1)
376-384:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winPartial-state risk if snapshot capture fails after the application is created.
preProcessAndAddApplicationpersists the application beforecaptureApplicationSnapshotruns. IfcaptureApplicationSnapshotthrowsAPIManagementException, the caller gets a 5xx but the application is already created without a governance snapshot — subsequent updates/key-gen flows for that app will then run against the "no snapshot" fallback path, which is silently different behavior. Worth at least catching the snapshot exception and either compensating (delete the just-created application) or downgrading to a logged warning so the create response stays consistent with what was persisted. Same consideration applies to theattachGovernanceSnapshotfallback at Lines 382–384 — if the snapshot write silently failed, falling back tobody.getTemplateId()will return a templateId that doesn't actually correspond to anything stored.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@components/apimgt/org.wso2.carbon.apimgt.rest.api.store.v1/src/main/java/org/wso2/carbon/apimgt/rest/api/store/v1/impl/ApplicationsApiServiceImpl.java` around lines 376 - 384, preProcessAndAddApplication persists the Application before DevportalGovernanceValidationUtil.captureApplicationSnapshot and attachGovernanceSnapshot run, risking a partial state if snapshot operations throw APIManagementException; wrap the snapshot calls (captureApplicationSnapshot and attachGovernanceSnapshot) in a try/catch that on failure either (a) performs a compensating delete of the created Application (use the createdApplication instance and existing delete API/manager) and rethrow a mapped error, or (b) if deletion is unsafe, catch APIManagementException, log an error with full details, set a flag on createdApplicationDTO to indicate snapshot-missing, and return a successful create response (avoid silently falling back to body.getTemplateId()); ensure the catch path does not leave the caller with a 5xx while the resource exists and that ApplicationDTO.templateId is only set to body.getTemplateId() when a snapshot was successfully stored or when explicitly documented as a fallback.components/apimgt/org.wso2.carbon.apimgt.rest.api.common/src/main/resources/governance-api.yaml-137-177 (1)
137-177:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winDocument
404for the template lookup/mutation endpoints.
/templates/defaultand/templates/{templateId}GET/PUT/DELETE can all surfaceDEVPORTAL_TEMPLATE_NOT_FOUND, but the spec only advertises success + generic errors. That leaves generated clients and API docs out of sync with actual behavior.Also applies to: 178-307
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@components/apimgt/org.wso2.carbon.apimgt.rest.api.common/src/main/resources/governance-api.yaml` around lines 137 - 177, Add a 404 response for template-not-found to the Devportal template endpoints: for /templates/default (operationId getDefaultDevportalGovernanceTemplate) and for all operations under /templates/{templateId} (the GET/PUT/DELETE operations), declare a "404" response with a description indicating DEVPORTAL_TEMPLATE_NOT_FOUND and include content application/json using the existing Error schema ($ref: '#/components/schemas/Error') so API docs and generated clients reflect the actual behavior.components/apimgt/org.wso2.carbon.apimgt.governance.rest.api/src/main/resources/governance-api.yaml-137-177 (1)
137-177:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winDocument
404for the template lookup/mutation endpoints.
/templates/defaultand/templates/{templateId}GET/PUT/DELETE can all surfaceDEVPORTAL_TEMPLATE_NOT_FOUND, but the spec only advertises success + generic errors. That leaves generated clients and API docs out of sync with actual behavior.Also applies to: 178-307
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@components/apimgt/org.wso2.carbon.apimgt.governance.rest.api/src/main/resources/governance-api.yaml` around lines 137 - 177, Add a 404 response to the template lookup/mutation endpoints to document DEVPORTAL_TEMPLATE_NOT_FOUND: for /templates/default (operationId getDefaultDevportalGovernanceTemplate) and the /templates/{templateId} GET/PUT/DELETE operations, add a "404" response entry with description like "Not Found. DEVPORTAL_TEMPLATE_NOT_FOUND" and content application/json using the existing Error schema ($ref: '#/components/schemas/Error') so generated clients/docs reflect the actual behavior.
Comment on lines
+549
to
+582
| List<DevportalGovernanceRulesetBinding> rulesetBindings = template.getRulesetBindings(); | ||
| for (DevportalGovernanceRulesetBinding binding : rulesetBindings) { | ||
| try (PreparedStatement prepStmt = | ||
| connection.prepareStatement(SQLConstants.CREATE_TEMPLATE_RULESET_BINDING)) { | ||
| prepStmt.setString(1, binding.getBindingId()); | ||
| prepStmt.setString(2, template.getId()); | ||
| prepStmt.setString(3, binding.getRulesetId()); | ||
| prepStmt.setInt(4, binding.getBindingOrder()); | ||
| prepStmt.setString(5, username); | ||
| prepStmt.setTimestamp(6, createdTime); | ||
| prepStmt.executeUpdate(); | ||
| } | ||
| addKeyManagerScopes(connection, binding, organization); | ||
| binding.setTemplateId(template.getId()); | ||
| binding.setCreatedBy(username); | ||
| binding.setCreatedTime(createdTime.toString()); | ||
| } | ||
| } | ||
|
|
||
| private void addKeyManagerScopes(Connection connection, DevportalGovernanceRulesetBinding binding, | ||
| String organization) throws SQLException { | ||
|
|
||
| if (binding.getKeyManagerScopes().isEmpty()) { | ||
| return; | ||
| } | ||
| try (PreparedStatement prepStmt = | ||
| connection.prepareStatement(SQLConstants.CREATE_TEMPLATE_RULESET_KM_SCOPE)) { | ||
| for (DevportalGovernanceKeyManagerScope keyManagerScope : binding.getKeyManagerScopes()) { | ||
| prepStmt.setString(1, binding.getBindingId()); | ||
| prepStmt.setString(2, keyManagerScope.getKeyManagerUuid()); | ||
| prepStmt.setString(3, organization); | ||
| prepStmt.addBatch(); | ||
| } | ||
| prepStmt.executeBatch(); |
There was a problem hiding this comment.
Optional collection fields can NPE template create/update.
Unscoped bindings are valid here, but this path assumes both template.getRulesetBindings() and each binding.getKeyManagerScopes() are non-null. If either list is omitted from the admin payload, template CRUD falls over with a NullPointerException before any validation runs.
🐛 Suggested fix
private void addRulesetBindings(Connection connection, DevportalGovernanceTemplate template, String organization,
String username, Timestamp createdTime) throws SQLException {
List<DevportalGovernanceRulesetBinding> rulesetBindings = template.getRulesetBindings();
+ if (rulesetBindings == null || rulesetBindings.isEmpty()) {
+ return;
+ }
for (DevportalGovernanceRulesetBinding binding : rulesetBindings) {
try (PreparedStatement prepStmt =
connection.prepareStatement(SQLConstants.CREATE_TEMPLATE_RULESET_BINDING)) {
@@
private void addKeyManagerScopes(Connection connection, DevportalGovernanceRulesetBinding binding,
String organization) throws SQLException {
- if (binding.getKeyManagerScopes().isEmpty()) {
+ List<DevportalGovernanceKeyManagerScope> keyManagerScopes = binding.getKeyManagerScopes();
+ if (keyManagerScopes == null || keyManagerScopes.isEmpty()) {
return;
}
try (PreparedStatement prepStmt =
connection.prepareStatement(SQLConstants.CREATE_TEMPLATE_RULESET_KM_SCOPE)) {
- for (DevportalGovernanceKeyManagerScope keyManagerScope : binding.getKeyManagerScopes()) {
+ for (DevportalGovernanceKeyManagerScope keyManagerScope : keyManagerScopes) {
prepStmt.setString(1, binding.getBindingId());
prepStmt.setString(2, keyManagerScope.getKeyManagerUuid());
prepStmt.setString(3, organization);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| List<DevportalGovernanceRulesetBinding> rulesetBindings = template.getRulesetBindings(); | |
| for (DevportalGovernanceRulesetBinding binding : rulesetBindings) { | |
| try (PreparedStatement prepStmt = | |
| connection.prepareStatement(SQLConstants.CREATE_TEMPLATE_RULESET_BINDING)) { | |
| prepStmt.setString(1, binding.getBindingId()); | |
| prepStmt.setString(2, template.getId()); | |
| prepStmt.setString(3, binding.getRulesetId()); | |
| prepStmt.setInt(4, binding.getBindingOrder()); | |
| prepStmt.setString(5, username); | |
| prepStmt.setTimestamp(6, createdTime); | |
| prepStmt.executeUpdate(); | |
| } | |
| addKeyManagerScopes(connection, binding, organization); | |
| binding.setTemplateId(template.getId()); | |
| binding.setCreatedBy(username); | |
| binding.setCreatedTime(createdTime.toString()); | |
| } | |
| } | |
| private void addKeyManagerScopes(Connection connection, DevportalGovernanceRulesetBinding binding, | |
| String organization) throws SQLException { | |
| if (binding.getKeyManagerScopes().isEmpty()) { | |
| return; | |
| } | |
| try (PreparedStatement prepStmt = | |
| connection.prepareStatement(SQLConstants.CREATE_TEMPLATE_RULESET_KM_SCOPE)) { | |
| for (DevportalGovernanceKeyManagerScope keyManagerScope : binding.getKeyManagerScopes()) { | |
| prepStmt.setString(1, binding.getBindingId()); | |
| prepStmt.setString(2, keyManagerScope.getKeyManagerUuid()); | |
| prepStmt.setString(3, organization); | |
| prepStmt.addBatch(); | |
| } | |
| prepStmt.executeBatch(); | |
| List<DevportalGovernanceRulesetBinding> rulesetBindings = template.getRulesetBindings(); | |
| if (rulesetBindings == null || rulesetBindings.isEmpty()) { | |
| return; | |
| } | |
| for (DevportalGovernanceRulesetBinding binding : rulesetBindings) { | |
| try (PreparedStatement prepStmt = | |
| connection.prepareStatement(SQLConstants.CREATE_TEMPLATE_RULESET_BINDING)) { | |
| prepStmt.setString(1, binding.getBindingId()); | |
| prepStmt.setString(2, template.getId()); | |
| prepStmt.setString(3, binding.getRulesetId()); | |
| prepStmt.setInt(4, binding.getBindingOrder()); | |
| prepStmt.setString(5, username); | |
| prepStmt.setTimestamp(6, createdTime); | |
| prepStmt.executeUpdate(); | |
| } | |
| addKeyManagerScopes(connection, binding, organization); | |
| binding.setTemplateId(template.getId()); | |
| binding.setCreatedBy(username); | |
| binding.setCreatedTime(createdTime.toString()); | |
| } | |
| } | |
| private void addKeyManagerScopes(Connection connection, DevportalGovernanceRulesetBinding binding, | |
| String organization) throws SQLException { | |
| List<DevportalGovernanceKeyManagerScope> keyManagerScopes = binding.getKeyManagerScopes(); | |
| if (keyManagerScopes == null || keyManagerScopes.isEmpty()) { | |
| return; | |
| } | |
| try (PreparedStatement prepStmt = | |
| connection.prepareStatement(SQLConstants.CREATE_TEMPLATE_RULESET_KM_SCOPE)) { | |
| for (DevportalGovernanceKeyManagerScope keyManagerScope : keyManagerScopes) { | |
| prepStmt.setString(1, binding.getBindingId()); | |
| prepStmt.setString(2, keyManagerScope.getKeyManagerUuid()); | |
| prepStmt.setString(3, organization); | |
| prepStmt.addBatch(); | |
| } | |
| prepStmt.executeBatch(); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@components/apimgt/org.wso2.carbon.apimgt.governance.impl/src/main/java/org/wso2/carbon/apimgt/governance/impl/dao/DevportalGovernanceDAO.java`
around lines 549 - 582, The code assumes template.getRulesetBindings() and
binding.getKeyManagerScopes() are non-null, causing NPEs; update the template
creation loop in DevportalGovernanceDAO (where rulesetBindings is assigned and
iterated) to first guard: if template.getRulesetBindings() is null treat it as
empty (e.g., skip the loop) and likewise modify addKeyManagerScopes(Connection,
DevportalGovernanceRulesetBinding, String) to return immediately if
binding.getKeyManagerScopes() is null or empty (currently only checks
isEmpty()); ensure you use a null-safe check before iterating and before
preparing/executing the batch so omitted collections in the payload won’t crash
template create/update.
Comment on lines
+442
to
+453
| private static DevportalGovernanceApplicationSnapshot getOrCaptureSnapshot(Application application, | ||
| String organization, Log log) | ||
| throws APIMGovernanceException, APIManagementException { | ||
|
|
||
| DevportalGovernanceApplicationSnapshot snapshot = DevportalGovernanceDAO.getInstance() | ||
| .getApplicationSnapshot(application.getId()); | ||
| if (snapshot != null) { | ||
| return snapshot; | ||
| } | ||
| return DevportalGovernanceDAO.getInstance().captureApplicationSnapshot(application.getId(), | ||
| application.getUUID(), null, resolveOrganization(application, organization)); | ||
| } |
There was a problem hiding this comment.
null snapshots leak into valid OAuth flows.
captureApplicationSnapshot(..., null, ...) can legitimately return null when an application has no stored snapshot and no default template is configured. The OAuth paths then dereference that result at Line 216, Line 269, and Line 421, so ungoverned applications hit a NullPointerException instead of simply skipping governance.
| then: | ||
| function: pattern | ||
| functionOptions: | ||
| notMatch: '(?i)^(test|sample|temp|untitled)[-_0-9]*$' |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In Stoplight Spectral, for the pattern function (match/notMatch), are inline regex modifiers like (?i) supported, or does it use JavaScript RegExp syntax that rejects this form?
💡 Result:
Inline regex modifiers like (?i) are not supported by Spectral’s pattern function (i.e., the match / notMatch options). Instead, Spectral expects JavaScript/ECMA-262 style regex delimiters and flags using the /pattern/flags form (e.g. "/^x-/i"). [1] Practical implication: using JavaScript regex literals with inline modifiers (like (?i)) or using modifier flags in a way Spectral can’t parse will fail; Spectral’s docs explicitly describe regex modifiers via delimiters such as "/[a-z]/i". [1] Support for case-insensitive matching is therefore done by supplying /.../i in match/notMatch, not (?i). This aligns with a documented change/issue where support for regex modifiers was added to the pattern function via providing delimiters/flags in the string (as illustrated by the docs using "/^x-/i"). [2][1]
Citations:
- 1: https://github.com/stoplightio/spectral/blob/develop/docs/reference/functions.md
- 2: [Feature] Pass pattern some regex modifiers as a function option stoplightio/spectral#2578
Inline (?i) regex flag is invalid for Spectral pattern function and will cause rule evaluation to fail.
Spectral requires JavaScript/ECMA-262 regex syntax with delimiters and flags (e.g., "/^pattern/i"), not inline modifiers. Use the correct format to enable case-insensitive matching.
Proposed fix
- notMatch: '(?i)^(test|sample|temp|untitled)[-_0-9]*$'
+ notMatch: '/^(test|sample|temp|untitled)[-_0-9]*/i'📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| notMatch: '(?i)^(test|sample|temp|untitled)[-_0-9]*$' | |
| notMatch: '/^(test|sample|temp|untitled)[-_0-9]*/i' |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@features/apimgt/org.wso2.carbon.apimgt.governance.feature/src/main/resources/default-rulesets/wso2_devportal_application_baseline.yaml`
at line 61, The YAML rule's notMatch value uses an inline (?i) flag which
Spectral's pattern function rejects; update the notMatch entry in
wso2_devportal_application_baseline.yaml to use a JavaScript/ECMA-262 regex
literal (with leading and trailing slashes) and the trailing i flag to make the
match case-insensitive, preserving the existing alternative group
(test|sample|temp|untitled) and the trailing [-_0-9]* quantifier so the
semantics remain identical.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Walkthrough Video
Feature walkthrough: Video
The walkthrough covers the complete flow across all three repositories:
Admin ruleset/template creation, server-backed form configuration, hidden default
validation, DevPortal template selection, application creation, snapshot-backed
OAuth governance, and product-level runtime behavior.
Summary:
This PR adds the backend foundation for DevPortal Governance. It introduces
admin-managed DevPortal Governance templates, APPLICATION ruleset support,
template/ruleset binding storage, application governance snapshots, REST APIs,
default rulesets, and synchronous runtime enforcement for DevPortal application
metadata and OAuth key/client lifecycle operations.
Scope:
Important behavior:
Documentation
devportal-governance-admin-guide.md
devportal-governance-architecture.md
devportal-governance-developer-guide.md
devportal-governance-docs-index.md
devportal-governance-qa-evidence.md
devportal-governance-rest-api-reference.md
devportal-governance-rule-authoring-guide.md
devportal-governance-upgrade-compatibility.md