refactor: update JSON marshaling to protojson in Postgres storage functions #2014
Conversation
WalkthroughThis pull request updates the API version from v1.3.0 to v1.3.1 across documentation, metadata, and version constants. It refines error handling by replacing fmt.Errorf with errors.New in select methods and streamlines function signatures by removing an unused parameter. Additionally, the pull request migrates JSON marshalling and unmarshalling from the deprecated jsonpb (and related imports) to protojson in several Postgres storage files, and enhances test assertions without altering core functionality. Changes
Possibly related PRs
Poem
✨ Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pkg/pb/base/v1/openapi.pb.gois excluded by!**/*.pb.go
📒 Files selected for processing (14)
docs/api-reference/apidocs.swagger.json(1 hunks)docs/api-reference/openapiv2/apidocs.swagger.json(1 hunks)internal/engines/check.go(1 hunks)internal/engines/entityFilter.go(1 hunks)internal/engines/subjectFilter.go(1 hunks)internal/engines/utils.go(0 hunks)internal/info.go(1 hunks)internal/storage/postgres/bundleReader.go(3 hunks)internal/storage/postgres/bundleWriter.go(2 hunks)internal/storage/postgres/dataReader.go(4 hunks)internal/storage/postgres/dataWriter.go(2 hunks)internal/storage/postgres/dataWriter_test.go(2 hunks)internal/storage/postgres/watch.go(2 hunks)proto/base/v1/openapi.proto(1 hunks)
💤 Files with no reviewable changes (1)
- internal/engines/utils.go
✅ Files skipped from review due to trivial changes (4)
- internal/info.go
- proto/base/v1/openapi.proto
- docs/api-reference/openapiv2/apidocs.swagger.json
- docs/api-reference/apidocs.swagger.json
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Test with Coverage
- GitHub Check: Analyze (go)
🔇 Additional comments (13)
internal/engines/check.go (1)
564-564: LGTM! Simplified error handling.The change from
fmt.Errorftoerrors.Newis appropriate since no string formatting is needed. This change improves code consistency and aligns with similar modifications in other files.internal/engines/subjectFilter.go (1)
340-340: LGTM! Consistent error handling.The change from
fmt.Errorftoerrors.Newmaintains consistency in error handling across the codebase and is appropriate since no string formatting is needed.internal/storage/postgres/bundleReader.go (2)
12-12: LGTM! Import ofprotojsonpackage.The import of
protojsonpackage is appropriate for the updated JSON unmarshaling implementation.
50-50: LGTM! Updated JSON unmarshaling implementation.The changes simplify the code by:
- Using shorthand declaration for
row.- Using
protojson.Unmarshaldirectly with byte slice instead of string reader.Also applies to: 61-62
internal/storage/postgres/bundleWriter.go (2)
10-10: LGTM! Import ofprotojsonpackage.The import of
protojsonpackage is appropriate for the updated JSON marshaling implementation.
46-47: LGTM! Updated JSON marshaling implementation.The changes improve the code by:
- Using
protojson.Marshalto convert to byte slice.- Converting byte slice to string for database storage.
Also applies to: 50-50
internal/storage/postgres/watch.go (2)
12-12: LGTM! Import ofprotojsonpackage.The import of
protojsonpackage is appropriate for the updated JSON unmarshaling implementation.
333-333: LGTM! Updated JSON unmarshaling implementation.The change to use
protojson.Unmarshalaligns with the standardized JSON handling approach.internal/storage/postgres/dataWriter_test.go (1)
490-490: LGTM! Enhanced test coverage.The additional error assertions improve test robustness by verifying that
ReadRelationshipsandReadAttributesoperations complete without errors.Also applies to: 500-500
internal/storage/postgres/dataWriter.go (2)
12-12: LGTM! Import changes for JSON marshaling.The import of
protojsonis correctly added for JSON marshaling.
493-499: LGTM! Migrated to protojson for JSON marshaling.The migration from
jsonpbtoprotojsonfor JSON marshaling is correct and follows best practices.Let's verify that the
protojsonpackage is being used consistently across the codebase:✅ Verification successful
🏁 Script executed:
#!/bin/bash # Description: Check for any remaining usage of jsonpb in the codebase. # Test: Search for any remaining imports of jsonpb. Expect: No results. rg -l 'github.com/golang/protobuf/jsonpb' # Test: Search for any remaining usage of jsonpb.Marshaler. Expect: No results. rg -l 'jsonpb\.Marshaler'Length of output: 69
Protojson usage verified across the codebase
• The search for remaining jsonpb imports and usage returned no results, confirming that no legacy jsonpb code remains.
• The migration to protojson for JSON marshaling is consistently applied, as evidenced by the absence of jsonpb-related code.internal/storage/postgres/dataReader.go (2)
12-12: LGTM! Import changes for JSON unmarshaling.The import of
protojsonis correctly added for JSON unmarshaling.
255-255: LGTM! Migrated to protojson for JSON unmarshaling.The migration from
jsonpbtoprotojsonfor JSON unmarshaling is correct and consistently applied across all functions.Let's verify that the
protojsonpackage is being used consistently across the codebase:Also applies to: 338-338, 433-433
✅ Verification successful
🏁 Script executed:
#!/bin/bash # Description: Check for any remaining usage of jsonpb in the codebase. # Test: Search for any remaining usage of jsonpb.Unmarshaler. Expect: No results. rg -l 'jsonpb\.Unmarshaler' # Test: Search for any remaining usage of strings.NewReader for JSON unmarshaling. Expect: No results. rg -l 'strings\.NewReader.*Unmarshal'Length of output: 69
Protojson migration confirmed across the codebase
The additional shell scripts confirmed that no usage of jsonpb (or its related JSON unmarshaling via strings.NewReader) remains. We also verified that protojson.Unmarshal is consistently applied in internal/storage/postgres/dataReader.go at lines 255, 338, and 433.
- No occurrences of jsonpb.Unmarshaler remain.
- No instances of strings.NewReader.*Unmarshal related to JSON unmarshaling are present.
- protojson is used as intended in all verified sections.
| } | ||
| case schema.AttributeLinkedEntrance: // If the linked entrance is a computed user set entrance. | ||
| err = engine.attributeEntrance(cont, request, entrance, visits, g, publisher) // Call the tuple to user set entrance method. | ||
| err = engine.attributeEntrance(cont, request, entrance, visits, publisher) // Call the tuple to user set entrance method. |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Verify concurrency requirements for attributeEntrance.
The removal of the errgroup.Group parameter from attributeEntrance creates an inconsistency in concurrency handling compared to other entrance types like relationEntrance and tupleToUserSetEntrance. This change might impact parallel processing capabilities.
Run the following script to analyze the usage patterns of errgroup.Group in entrance methods:
🏁 Script executed:
#!/bin/bash
# Description: Analyze errgroup.Group usage in entrance methods
# Test: Search for entrance methods and their errgroup.Group usage
# Search for entrance method signatures
echo "Entrance method signatures:"
ast-grep --pattern $'func \(engine \*EntityFilter\) $_Entrance\(.*\) error'
# Search for errgroup.Group usage within entrance methods
echo -e "\nUsage of errgroup.Group within entrance methods:"
rg -A 5 'func \(engine \*EntityFilter\) \w+Entrance\(' | rg -A 5 'g \*errgroup\.Group'Length of output: 746
Concurrency inconsistency in attributeEntrance
The analysis confirmed that while methods like tupleToUserSetEntrance still reference an errgroup (as seen by the declaration “g *errgroup.Group”), attributeEntrance no longer accepts or uses an errgroup.Group parameter. This difference in how concurrency is handled across entrance methods may affect parallel processing capabilities. Consider revisiting attributeEntrance to determine whether it should incorporate an errgroup for consistent concurrency management or if its behavior is intentionally sequential.
Summary by CodeRabbit
This release standardizes our API version to v1.3.1 while improving internal efficiency and reliability—all with no impact on public API behavior.