fix(jans-config-api): save usageType field for /api/v1/attributes#13571
Open
pradhankukiran wants to merge 1 commit intoJanssenProject:mainfrom
Open
fix(jans-config-api): save usageType field for /api/v1/attributes#13571pradhankukiran wants to merge 1 commit intoJanssenProject:mainfrom
pradhankukiran wants to merge 1 commit intoJanssenProject:mainfrom
Conversation
pradhankukiran
requested review from
devrimyatar,
iromli,
pujavs,
yurem,
yuriyz and
yuriyzz
as code owners
Member
✅ Snyk checks have passed. No issues have been found so far.
💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse. |
Contributor
📝 WalkthroughWalkthroughAdds support for a multivalued Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@jans-config-api/server/src/test/java/io/jans/configapi/test/auth/AttributesResourceTest.java`:
- Around line 38-39: The tests in AttributesResourceTest currently mutate a
shared built-in attribute (ATTRIBUTE_NAME = "departmentNumber") which can cause
cross-test interference; change the tests to use a dedicated test-only attribute
name (e.g., generate a unique name or use a constant like TEST_ATTRIBUTE_NAME)
instead of ATTRIBUTE_NAME and update any fixtures that reference
USAGE_TYPE_PATCH to patch that test-only attribute; ensure the test creates the
attribute in setup and fully cleans it up in teardown (or mocks the attribute
store) so methods like the JSON patch constant USAGE_TYPE_PATCH and any
create/update/delete helpers operate only on the newly created test attribute
and not on built-in attributes.
- Around line 69-71: The finally block currently calls putAttribute(issuer,
attributesUrl, restoreAttribute, objectMapper) directly which can override an
earlier test failure; change the pattern in AttributesResourceTest so you
capture any primary exception from the try block (e.g., store Throwable primary
= null; inside catch assign primary = t; rethrow) and in the finally call
putAttribute inside its own try/catch; if the restore throws, call
primary.addSuppressed(restoreException) and rethrow the original primary (or if
primary is null, rethrow the restore exception) so the original assertion/HTTP
failure is preserved while still recording the restore error.
- Around line 86-88: In AttributesResourceTest, Response objects created via
request.get() (the local variable response) are not closed; wrap each usage in a
try-with-resources block like try (Response response = request.get()) { String
responseBody = response.readEntity(String.class); ... } (or call
response.close() after reading) at each place response is created so JAX-RS
connections are released and the client pool is not leaked.
In
`@jans-config-api/shared/src/main/java/io/jans/configapi/core/util/Jackson.java`:
- Around line 107-109: The current round-trip uses
ObjectMapperContextResolver.createDefaultMapper() which strips NON_EMPTY
properties and rewrites Dates; change the mapper used for the POJO → JsonNode →
POJO path so the round-trip is lossless: create or obtain a plain ObjectMapper
configured to include ALL properties and use the default date handling (e.g.,
new ObjectMapper() or a copy of the default config with
JsonInclude.Include.ALWAYS and standard DateFormat) and use that mapper for
objectMapper.convertValue(obj, JsonNode.class), jsonPatch.apply(...) and
objectMapper.treeToValue(...), leaving
ObjectMapperContextResolver.createDefaultMapper() only for API payload shaping.
Ensure calls referencing convertValue and treeToValue in Jackson.java use this
lossless mapper.
In `@jans-linux-setup/jans_setup/setup_app/installers/rdbm.py`:
- Around line 372-397: The migration currently converts NULL to an empty array;
update the CASE expressions used in both branches so NULL values are preserved
(remain NULL) while only empty strings become empty JSON arrays. In the pgsql
branch change the USING clause in the ALTER to: CASE WHEN "{1}" IS NULL THEN
NULL WHEN "{1}" = '' THEN '[]'::jsonb ELSE jsonb_build_array("{1}") END; and in
the non-pgsql/MySQL branch change the UPDATE CASE to: CASE WHEN `{2}` IS NULL
THEN NULL WHEN `{2}` = '' THEN JSON_ARRAY() ELSE JSON_ARRAY(`{2}`) END;. Keep
references to sql_cmds, attrname, table_name and temp_column and ensure
column_exists logic remains unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: c0ef2fe0-10a4-4043-87a7-2071d2941525
📒 Files selected for processing (14)
docs/janssen-server/config-guide/auth-server-config/attribute-configuration.mddocs/janssen-server/reference/database/mysql-schema.mddocs/janssen-server/reference/database/pgsql-schema.mdjans-config-api/docs/jans-config-api-swagger.yamljans-config-api/server/src/main/resources/example/attribute/attribute.jsonjans-config-api/server/src/main/resources/example/database/tableInfo.jsonjans-config-api/server/src/test/java/io/jans/configapi/test/auth/AttributesResourceTest.javajans-config-api/server/src/test/java/io/jans/configapi/test/util/JacksonPatchTest.javajans-config-api/server/src/test/resources/json/attribute/attribute.jsonjans-config-api/server/src/test/resources/json/attribute/attributes.featurejans-config-api/server/src/test/resources/testng.xmljans-config-api/shared/src/main/java/io/jans/configapi/core/util/Jackson.javajans-linux-setup/jans_setup/schema/jans_schema.jsonjans-linux-setup/jans_setup/setup_app/installers/rdbm.py
Comment on lines
+38
to
+39
| private static final String ATTRIBUTE_NAME = "departmentNumber"; | ||
| private static final String USAGE_TYPE_PATCH = "[{\"op\":\"add\",\"path\":\"/usageType\",\"value\":[\"openid\"]}]"; |
Contributor
There was a problem hiding this comment.
Avoid mutating a shared built-in attribute (departmentNumber) in integration tests.
Using a common pre-existing attribute as mutable test fixture can cause cross-test interference (especially with parallel runs) and non-deterministic failures even with restore logic.
Also applies to: 43-71
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@jans-config-api/server/src/test/java/io/jans/configapi/test/auth/AttributesResourceTest.java`
around lines 38 - 39, The tests in AttributesResourceTest currently mutate a
shared built-in attribute (ATTRIBUTE_NAME = "departmentNumber") which can cause
cross-test interference; change the tests to use a dedicated test-only attribute
name (e.g., generate a unique name or use a constant like TEST_ATTRIBUTE_NAME)
instead of ATTRIBUTE_NAME and update any fixtures that reference
USAGE_TYPE_PATCH to patch that test-only attribute; ensure the test creates the
attribute in setup and fully cleans it up in teardown (or mocks the attribute
store) so methods like the JSON patch constant USAGE_TYPE_PATCH and any
create/update/delete helpers operate only on the newly created test attribute
and not on built-in attributes.
Comment on lines
+69
to
+71
| } finally { | ||
| putAttribute(issuer, attributesUrl, restoreAttribute, objectMapper); | ||
| } |
Contributor
There was a problem hiding this comment.
Preserve primary test failure when restore in finally fails.
At Line 70, an exception from restore PUT can mask the original assertion/HTTP failure from the test body, reducing diagnosability.
🛠️ Suggested pattern
+ Throwable primaryFailure = null;
try {
...
+ } catch (Throwable t) {
+ primaryFailure = t;
+ throw t;
} finally {
- putAttribute(issuer, attributesUrl, restoreAttribute, objectMapper);
+ try {
+ putAttribute(issuer, attributesUrl, restoreAttribute, objectMapper);
+ } catch (Exception restoreEx) {
+ if (primaryFailure != null) {
+ primaryFailure.addSuppressed(restoreEx);
+ } else {
+ throw restoreEx;
+ }
+ }
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@jans-config-api/server/src/test/java/io/jans/configapi/test/auth/AttributesResourceTest.java`
around lines 69 - 71, The finally block currently calls putAttribute(issuer,
attributesUrl, restoreAttribute, objectMapper) directly which can override an
earlier test failure; change the pattern in AttributesResourceTest so you
capture any primary exception from the try block (e.g., store Throwable primary
= null; inside catch assign primary = t; rethrow) and in the finally call
putAttribute inside its own try/catch; if the restore throws, call
primary.addSuppressed(restoreException) and rethrow the original primary (or if
primary is null, rethrow the restore exception) so the original assertion/HTTP
failure is preserved while still recording the restore error.
Signed-off-by: KIRAN <pradhankukiran@gmail.com>
pradhankukiran
force-pushed
the
jans-config-api-fix-usagetype
branch
from
da51ce6 to
5a9d3f5
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
jans-config-api/server/src/test/java/io/jans/configapi/test/auth/AttributesResourceTest.java (2)
38-46:⚠️ Potential issue | 🟠 MajorAvoid mutating shared built-in attribute state in this integration test.
Using
departmentNumberas a mutable fixture can cause cross-test interference and non-deterministic failures (especially with parallel runs or retries). Use a dedicated test-only attribute lifecycle (create/update/delete) instead of modifying a built-in global attribute.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@jans-config-api/server/src/test/java/io/jans/configapi/test/auth/AttributesResourceTest.java` around lines 38 - 46, The test putAndPatchUsageTypeRoundTrip currently mutates the shared built-in attribute named by ATTRIBUTE_NAME ("departmentNumber") via getAttributeByName and copyAttribute; change the test to create a dedicated test attribute (use the API helper that creates attributes or add a new helper like createTestAttribute with a unique name), operate on that created attribute in getAttributeByName/copyAttribute/patch flows, and ensure teardown deletes the test attribute (deleteTestAttribute) so built-in attributes are never modified; update references in putAndPatchUsageTypeRoundTrip to use the new test attribute name and ensure cleanup runs even on failure.
69-71:⚠️ Potential issue | 🟡 MinorPreserve primary test failure when restore fails in
finally.The restore
putAttribute(...)at Line 70 can override the original assertion/HTTP failure from the test body. Capture primary throwable and suppress restore errors onto it so diagnostics are preserved.Suggested fix
- try { + Throwable primaryFailure = null; + try { JansAttribute putRequest = copyAttribute(objectMapper, originalAttribute); putRequest.setUsageType(new GluuAttributeUsageType[] { GluuAttributeUsageType.OPENID }); @@ JansAttribute fetchedAfterPatch = getAttributeByInum( issuer, attributesUrl, originalAttribute.getInum(), objectMapper); assertUsageType(fetchedAfterPatch); + } catch (Throwable t) { + primaryFailure = t; + throw t; } finally { - putAttribute(issuer, attributesUrl, restoreAttribute, objectMapper); + try { + putAttribute(issuer, attributesUrl, restoreAttribute, objectMapper); + } catch (Exception restoreEx) { + if (primaryFailure != null) { + primaryFailure.addSuppressed(restoreEx); + } else { + throw restoreEx; + } + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@jans-config-api/server/src/test/java/io/jans/configapi/test/auth/AttributesResourceTest.java` around lines 69 - 71, The finally block calling putAttribute(issuer, attributesUrl, restoreAttribute, objectMapper) can swallow the original test failure; change the test to capture any primary Throwable from the try/catch (e.g., assign to a local primaryThrowable when an exception is caught or rethrown) and in the finally wrap the putAttribute call in its own try/catch: if putAttribute throws and primaryThrowable is non-null, add the restore exception as suppressed via primaryThrowable.addSuppressed(restoreEx), otherwise rethrow the restore exception; reference the test method in AttributesResourceTest and the putAttribute(...) call to locate and update the finally logic.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@jans-linux-setup/jans_setup/setup_app/installers/rdbm.py`:
- Around line 393-423: The code currently appends an 'ALTER TABLE ... DROP
COLUMN `{attrname}`' into sql_cmds before the subsequent 'ALTER TABLE ... CHANGE
COLUMN `{temp_column}` `{attrname}` ...' operations, risking loss if the CHANGE
fails; remove the DROP from the initial sql_cmds.extend block and instead append
the DROP after you build the CHANGE COLUMN statement(s) (the block that uses
get_attr_description / escaped_desc) so the sequence is: add temp column, copy
data into temp_column, perform CHANGE COLUMN `{temp_column}` -> `{attrname}`
(with comment if desc exists), and only then append 'ALTER TABLE ... DROP COLUMN
`{old_column_name}`' to sql_cmds; update references to temp_column, attrname,
sql_cmds, get_attr_description and escaped_desc accordingly.
---
Duplicate comments:
In
`@jans-config-api/server/src/test/java/io/jans/configapi/test/auth/AttributesResourceTest.java`:
- Around line 38-46: The test putAndPatchUsageTypeRoundTrip currently mutates
the shared built-in attribute named by ATTRIBUTE_NAME ("departmentNumber") via
getAttributeByName and copyAttribute; change the test to create a dedicated test
attribute (use the API helper that creates attributes or add a new helper like
createTestAttribute with a unique name), operate on that created attribute in
getAttributeByName/copyAttribute/patch flows, and ensure teardown deletes the
test attribute (deleteTestAttribute) so built-in attributes are never modified;
update references in putAndPatchUsageTypeRoundTrip to use the new test attribute
name and ensure cleanup runs even on failure.
- Around line 69-71: The finally block calling putAttribute(issuer,
attributesUrl, restoreAttribute, objectMapper) can swallow the original test
failure; change the test to capture any primary Throwable from the try/catch
(e.g., assign to a local primaryThrowable when an exception is caught or
rethrown) and in the finally wrap the putAttribute call in its own try/catch: if
putAttribute throws and primaryThrowable is non-null, add the restore exception
as suppressed via primaryThrowable.addSuppressed(restoreEx), otherwise rethrow
the restore exception; reference the test method in AttributesResourceTest and
the putAttribute(...) call to locate and update the finally logic.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: a476ec7c-1284-41a7-ba6a-d778773b3a49
📒 Files selected for processing (14)
docs/janssen-server/config-guide/auth-server-config/attribute-configuration.mddocs/janssen-server/reference/database/mysql-schema.mddocs/janssen-server/reference/database/pgsql-schema.mdjans-config-api/docs/jans-config-api-swagger.yamljans-config-api/server/src/main/resources/example/attribute/attribute.jsonjans-config-api/server/src/main/resources/example/database/tableInfo.jsonjans-config-api/server/src/test/java/io/jans/configapi/test/auth/AttributesResourceTest.javajans-config-api/server/src/test/java/io/jans/configapi/test/util/JacksonPatchTest.javajans-config-api/server/src/test/resources/json/attribute/attribute.jsonjans-config-api/server/src/test/resources/json/attribute/attributes.featurejans-config-api/server/src/test/resources/testng.xmljans-config-api/shared/src/main/java/io/jans/configapi/core/util/Jackson.javajans-linux-setup/jans_setup/schema/jans_schema.jsonjans-linux-setup/jans_setup/setup_app/installers/rdbm.py
Comment on lines
+393
to
+423
| sql_cmds.extend( | ||
| [ | ||
| 'ALTER TABLE `{}` ADD COLUMN `{}` JSON;'.format(table_name, temp_column), | ||
| ( | ||
| "UPDATE `{0}` SET `{1}` = CASE " | ||
| "WHEN `{2}` IS NULL THEN NULL " | ||
| "WHEN `{2}` = '' THEN JSON_ARRAY() " | ||
| "ELSE JSON_ARRAY(`{2}`) END;" | ||
| ).format(table_name, temp_column, attrname), | ||
| 'ALTER TABLE `{}` DROP COLUMN `{}`;'.format(table_name, attrname), | ||
| ] | ||
| ) | ||
|
|
||
| desc = self.get_attr_description(attrname) | ||
| if desc: | ||
| escaped_desc = desc.replace('\\', '\\\\').replace('"', '\\"') | ||
| sql_cmds.append( | ||
| 'ALTER TABLE `{0}` CHANGE COLUMN `{1}` `{2}` JSON COMMENT "{3}";'.format( | ||
| table_name, | ||
| temp_column, | ||
| attrname, | ||
| escaped_desc, | ||
| ) | ||
| ) | ||
| else: | ||
| sql_cmds.append( | ||
| 'ALTER TABLE `{0}` CHANGE COLUMN `{1}` `{2}` JSON;'.format( | ||
| table_name, | ||
| temp_column, | ||
| attrname, | ||
| ) |
Contributor
There was a problem hiding this comment.
Keep the old column until the rename has succeeded.
Line 402 drops attrname before Lines 410-423 recreate that name on the temp column. If the later CHANGE COLUMN fails, the upgrade is left without the expected column name and the copied data only survives under *_tmp_json.
🧰 Tools
🪛 Ruff (0.15.6)
[warning] 395-395: Use f-string instead of format call
Convert to f-string
(UP032)
[warning] 396-401: Use f-string instead of format call
Convert to f-string
(UP032)
[error] 396-401: Possible SQL injection vector through string-based query construction
(S608)
[warning] 402-402: Use f-string instead of format call
Convert to f-string
(UP032)
[warning] 410-415: Use implicit references for positional format fields
Remove explicit positional indices
(UP030)
[warning] 410-415: Use f-string instead of format call
Convert to f-string
(UP032)
[warning] 419-423: Use implicit references for positional format fields
Remove explicit positional indices
(UP030)
[warning] 419-423: Use f-string instead of format call
Convert to f-string
(UP032)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@jans-linux-setup/jans_setup/setup_app/installers/rdbm.py` around lines 393 -
423, The code currently appends an 'ALTER TABLE ... DROP COLUMN `{attrname}`'
into sql_cmds before the subsequent 'ALTER TABLE ... CHANGE COLUMN
`{temp_column}` `{attrname}` ...' operations, risking loss if the CHANGE fails;
remove the DROP from the initial sql_cmds.extend block and instead append the
DROP after you build the CHANGE COLUMN statement(s) (the block that uses
get_attr_description / escaped_desc) so the sequence is: add temp column, copy
data into temp_column, perform CHANGE COLUMN `{temp_column}` -> `{attrname}`
(with comment if desc exists), and only then append 'ALTER TABLE ... DROP COLUMN
`{old_column_name}`' to sql_cmds; update references to temp_column, attrname,
sql_cmds, get_attr_description and escaped_desc accordingly.
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
2 participants
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.
Prepare
Description
Target issue
closes #13517
Implementation Details
The
usageTypefield was not persisting onPOST/PUT/PATCH/api/v1/attributesbecause thejansAttrUsgTypdatabase column wasvarchar(64)while the Java model declares it as a multi-valued array (GluuAttributeUsageType[]).jansAttrUsgTypasmultivaluedinjans_schema.jsonvarcharcolumns tojson/jsonbfor both MySQL and PostgreSQLJackson.applyPatchto use the APIObjectMapperso enums deserialize correctly on PATCHTest and Document the changes
Please check the below before submitting your PR. The PR will not be merged if there are no commits that start with
docs:to indicate documentation changes or if the below checklist is not selected.Summary by CodeRabbit
New Features
Documentation
Tests
Chores