-
Notifications
You must be signed in to change notification settings - Fork 2
feat: enhance identifier handling with new generated model #64
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
feat: enhance identifier handling with new generated model #64
Conversation
Sotatek-DucPhung
commented
Dec 1, 2025
- Introduced IdentifierModelConverter for converting generated KERIA models to domain models.
- Added IdentifierPayloadMapper to build request payloads for identifier endpoints.
- Updated Identifier class to utilize GeneratedModelMapper for JSON parsing.
- Modified IdentifierListResponse to use a list of Identifier objects instead of generic Object.
- Implemented KeyStateRecordDeserializer to handle deserialization of KeyStateRecord.
- Centralized Jackson configuration in GeneratedModelConfig for better management of generated models.
- Updated various tests to reflect changes in identifier handling and ensure consistency.
- Introduced IdentifierModelConverter for converting generated KERIA models to domain models. - Added IdentifierPayloadMapper to build request payloads for identifier endpoints. - Updated Identifier class to utilize GeneratedModelMapper for JSON parsing. - Modified IdentifierListResponse to use a list of Identifier objects instead of generic Object. - Implemented KeyStateRecordDeserializer to handle deserialization of KeyStateRecord. - Centralized Jackson configuration in GeneratedModelConfig for better management of generated models. - Updated various tests to reflect changes in identifier handling and ensure consistency.
src/main/java/org/cardanofoundation/signify/app/config/GeneratedModelMapper.java
Outdated
Show resolved
Hide resolved
| ObjectNode node = mapper.readTree(p); | ||
|
|
||
| coerceToString(node, "kt"); | ||
| coerceToString(node, "nt"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we really ever get ints back from the API? I thought it was always strings. (I know the API for creating an identifier accepts both though, which is different)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We hit a parse failure when kt/nt came back as an array (rotation thresholds), so this coerces non-text (array/number) to a string before binding.
If the API is guaranteed to return strings only, I can tighten this to handle only arrays (or drop the coercion and let it fail loudly). Let me know your prefer
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah yes, but it's an array of strings. I think @Sotatek-Patrick-Vu might be fixing this, or have fixed it. We should return the string array, not a string.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
updated in coerceToString method
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right but is it possible to receive a numeric response from the API? I don't think it is (may be a mistake in Patrick's generated models)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Anw, i have add logic to handle the String array deserilizer, so the e2e test is passed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is incorrect, we are not meant to have strings of JSON arrays. We are meant to have arrays.
Please work with Patrick to get the OpenAPI changes needed to update the generated model and have the correct format.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Sotatek-Patrick-Vu could you please take a look for these field ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We are expecting kt, nt properties are string or array string or array of array string But It seems we cannot generate correct in java
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Sotatek-DucPhung @Sotatek-PhongVu @Sotatek-Patrick-Vu It's also not generated correctly in Signify-TS: https://github.com/WebOfTrust/signify-ts/blob/main/src/types/keria-api-schema.ts#L363
This is referring to the Key State only.
So I think there are missing changes in KERIA to generate this correctly in both Signify-TS and Signify-Java: cardano-foundation/keria#16
src/main/java/org/cardanofoundation/signify/app/aiding/IdentifierPayloadMapper.java
Outdated
Show resolved
Hide resolved
src/main/java/org/cardanofoundation/signify/app/aiding/IdentifierPayloadMapper.java
Outdated
Show resolved
Hide resolved
src/main/java/org/cardanofoundation/signify/app/aiding/IdentifierModelConverter.java
Outdated
Show resolved
Hide resolved
src/main/java/org/cardanofoundation/signify/app/config/GeneratedModelMapper.java
Outdated
Show resolved
Hide resolved
src/main/java/org/cardanofoundation/signify/app/aiding/Identifier.java
Outdated
Show resolved
Hide resolved
src/main/java/org/cardanofoundation/signify/app/aiding/IdentifierPayloadMapper.java
Outdated
Show resolved
Hide resolved
src/main/java/org/cardanofoundation/signify/app/aiding/IdentifierPayloadMapper.java
Outdated
Show resolved
Hide resolved
src/main/java/org/cardanofoundation/signify/app/aiding/IdentifierModelConverter.java
Outdated
Show resolved
Hide resolved
…ds and using direct calls
src/main/java/org/cardanofoundation/signify/app/aiding/IdentifierModelConverter.java
Outdated
Show resolved
Hide resolved
src/main/java/org/cardanofoundation/signify/app/aiding/IdentifierPayloadMapper.java
Outdated
Show resolved
Hide resolved
src/main/java/org/cardanofoundation/signify/app/aiding/IdentifierModelConverter.java
Outdated
Show resolved
Hide resolved
…hods for consistency
6666b59 to
c099e48
Compare
…onsistency and clarity
…ier-model refactor: replace States with Tier in multiple classes for improved c…
src/main/java/org/cardanofoundation/signify/app/aiding/IdentifierPayloadMapper.java
Outdated
Show resolved
Hide resolved
| ObjectNode node = mapper.readTree(p); | ||
|
|
||
| coerceToString(node, "kt"); | ||
| coerceToString(node, "nt"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Still pending a response here!
src/main/java/org/cardanofoundation/signify/app/config/GeneratedModelMapper.java
Outdated
Show resolved
Hide resolved
src/test/java/org/cardanofoundation/signify/app/RegistryTest.java
Outdated
Show resolved
Hide resolved
src/main/java/org/cardanofoundation/signify/app/aiding/IdentifierPayloadMapper.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pointer to https://github.com/cardano-foundation/cf-signify-java/pull/59/files#r2604004041
Should be resolved before this PR is merged!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Sotatek-DucPhung Please check my comment there: #59 (comment)
Those changes MUST be fixed in this PR before merging.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If it will take time to fix those, and we resolve the other comment in this thread first. We could defer fixing these until the next PR
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yep, sounds good to me
|
@iFergal please take a look again! Thanks |
src/main/java/org/cardanofoundation/signify/app/aiding/IdentifierController.java
Outdated
Show resolved
Hide resolved
| return Optional.empty(); | ||
| } | ||
|
|
||
| // Note: KERIA can return either a single object or an array |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't believe this is true:
states = []
for pre in pres:
if pre not in agent.hby.kevers:
continue
kever = agent.hby.kevers[pre]
states.append(asdict(kever.state()))
rep.status = falcon.HTTP_200
rep.content_type = "application/json"
rep.data = json.dumps(states).encode("utf-8")
In fact, this endpoint could just take pre and call .list([pre]) or something
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
so this keria endpoint return an array right ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep. For both .get and .list it will return a list from the endpoint. If .get, it will be an array of length 1 at most.
(It could be [] if you call .get with an AID that KERIA does not know about (the continue line above))
| return Optional.of(Utils.fromJson(res.body(), Object.class)); | ||
|
|
||
| String body = res.body(); | ||
| if (body == null || body.isBlank()) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It will always at least return an empty array
| ObjectNode node = mapper.readTree(p); | ||
|
|
||
| coerceToString(node, "kt"); | ||
| coerceToString(node, "nt"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is incorrect, we are not meant to have strings of JSON arrays. We are meant to have arrays.
Please work with Patrick to get the OpenAPI changes needed to update the generated model and have the correct format.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Sotatek-DucPhung Please check my comment there: #59 (comment)
Those changes MUST be fixed in this PR before merging.