refactor: make Statement.Value a json.RawMessage to get rid of custom (un)marshallers #1936
Conversation
dschmidt
force-pushed
the
feat/statement-json-marshaler
branch
from
f1ddd10 to
3a64a2a
Compare
|
I've updated the PR with a change to Assertion.GetHash()` because after an update of the SDK the verification of the assertion signing failed. Tbh it feels even messier now than before. |
|
/gemini review |
This makes a lot of sense. Do you want to take that change otherwise I'll add it to our backlog. |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a custom MarshalJSON for sdk.Statement to handle structured JSON values, improving consistency with the existing UnmarshalJSON. I've suggested removing the redundant "deep patch" in GetHash and refactoring the logic in MarshalJSON to be more concise. The approach of falling back to treating the value as a string if it's not valid JSON is a good, robust choice. Remember to add unit tests to cover the new marshaling logic.
sdk/assertion.go
Outdated
| // Deep patch: fix "value" inside "statement" if it is a string | ||
| if statement, ok := jsonObject["statement"].(map[string]interface{}); ok { | ||
| if valueStr, ok := statement["value"].(string); ok { | ||
| var valueObj interface{} | ||
| if err := json.Unmarshal([]byte(valueStr), &valueObj); err == nil { | ||
| statement["value"] = valueObj // replace the string with parsed object | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
This "deep patch" in GetHash might be redundant given the new MarshalJSON implementation for Statement.
The GetHash function starts by calling json.Marshal(a). This will invoke the custom MarshalJSON for the a.Statement field. The new marshaler is designed to handle the json-structured format by outputting the value as a JSON object if it's a valid JSON string.
As a result, when assertionJSON is unmarshaled into jsonObject, jsonObject["statement"]["value"] should already be a map[string]interface{}, not a string. This means the condition if valueStr, ok := statement["value"].(string); ok will likely be false, and this block of code will not be executed.
Consider removing this patch to simplify the code. If it's intended as a safeguard, add a comment explaining the specific scenario it covers. Otherwise, it may be unnecessary.
sdk/assertion.go
Outdated
| if s.Format == "json-structured" { | ||
| raw := json.RawMessage(s.Value) | ||
| var tmp interface{} | ||
| // Attempt to decode Value to validate it's actual json | ||
| if err := json.Unmarshal(raw, &tmp); err == nil { | ||
| aux.Value = raw | ||
| } else { | ||
| aux.Value = s.Value | ||
| } | ||
| } else { | ||
| aux.Value = s.Value | ||
| } |
There was a problem hiding this comment.
The logic can be simplified to reduce redundancy and improve readability. The else branch for a failed JSON unmarshal and the else branch for a non-json-structured format both perform the same action: aux.Value = s.Value.
Consider setting s.Value as the default for aux.Value, and then conditionally overriding it with a json.RawMessage only if the format is json-structured and the value is valid JSON.
aux.Value = s.Value
if s.Format == "json-structured" {
var tmp interface{}
// If Value is a valid JSON, marshal it as a raw JSON object.
if json.Unmarshal([]byte(s.Value), &tmp) == nil {
aux.Value = json.RawMessage(s.Value)
}
}
dschmidt
force-pushed
the
feat/statement-json-marshaler
branch
3 times, most recently
from
83142ee to
c735604
Compare
dschmidt
force-pushed
the
feat/statement-json-marshaler
branch
from
c735604 to
19f2093
Compare
dschmidt
changed the title
|
@jrschumacher ping? :) |
|
@dschmidt we're reviewing this with some additional changes to assertions that are coming. We should be ready for a decision this week. |
|
Alright, sounds perfect! Happy this isn't forgotten :) |
|
So .. any update? |
|
Draft PR here: #2687 There is a WIP ADR. It will hold the decision on this refactor. We are heavily considering keeping the |
4 participants
Proposed Changes
If you agree with this change, I'm happy to add tests.
I assume it's a good idea to try to decode the value to make sure it's actual json. I just wasn't sure whether we should return an error if not or if we should just use the value as string.
Opinions?
Checklist
Testing Instructions