Skip to content
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

Add useDeterministicOrdering opt-in option to JSON + Binary encoding #1487

Merged

Conversation

rebello95
Copy link
Contributor

Cherry-picks #1478 and #1480 from main onto the 1.x branch. Includes various fixes because these were not clean cherry picks.

* Add `useDeterministicOrdering` to `JSONEncodingOptions`

Adds an option to ensure that JSON serialization is deterministic when serializing Protobuf `map` fields. This should be the only type that needs to be sorted, since individual fields are serialized in order by the generated code that invokes `try visitor.visit(...)` for each field.

Fixes apple#1477
…pple#1480)

* Add `BinaryEncodingOptions` & option for deterministic ordering

Implements the same setting added for JSON in apple#1478 for binary serialization.

Related to apple#1477.
@rebello95 rebello95 marked this pull request as ready for review October 26, 2023 18:43
@rebello95
Copy link
Contributor Author

I don't understand the test failure - looks like make generate-linux-main isn't including one of the new test cases? @thomasvl

@thomasvl
Copy link
Collaborator

I don't understand the test failure - looks like make generate-linux-main isn't including one of the new test cases? @thomasvl

@tbkka wrote the test case collector we used back when SwiftPM didn't have native support to do the work for Linux and can hopefully provide some info.

My guess is this line doesn't deal with the final class usage, try switching to just class.

@rebello95
Copy link
Contributor Author

My guess is this line doesn't deal with the final class usage, try switching to just class.

Ah yea that makes sense. I'll remove final for now

@thomasvl thomasvl requested a review from tbkka October 26, 2023 20:31
@tbkka
Copy link
Collaborator

tbkka commented Oct 26, 2023

#1488 fixes the problem with having final test classes in the 1.x branch. (The main branch doesn't need to use a script for test discovery, so doesn't need this fix.)

@rebello95
Copy link
Contributor Author

Thanks @tbkka! If we land this one, it could be a good test of #1488 to add final to all the test classes to validate it as well

@thomasvl
Copy link
Collaborator

Thank you for doing the work to pull it to 1.x!

@thomasvl thomasvl merged commit 6a28e89 into apple:1_x_release_branch Oct 27, 2023
17 checks passed
@rebello95 rebello95 deleted the deterministic-ordering-1x branch October 27, 2023 16:45
@rebello95
Copy link
Contributor Author

Thank you for doing the work to pull it to 1.x!

🙌🏽 do you know when a new release is expected for 1.x?

@thomasvl
Copy link
Collaborator

@tbkka make sense to cut a release? Change since last release:

  • The plugin refactoring
  • These two new encoding options
  • CI for Swift 5.9

@tbkka
Copy link
Collaborator

tbkka commented Oct 27, 2023

I think it's reasonable.

@rebello95
Copy link
Contributor Author

Thank you!

rebello95 added a commit to connectrpc/connect-swift that referenced this pull request Nov 13, 2023
## Summary

Implements support for Connect unary GET requests per
https://connectrpc.com/docs/protocol#unary-get-request.

By default, this behavior is disabled and can be enabled through an
option on `ProtocolClientConfig` (as demonstrated in the Eliza app).
Behavior is exercised in a new conformance test, but I do intend to add
more unit tests for this after #208.

## Linked issues

- Requires apple/swift-protobuf#1487
- Related to apple/swift-protobuf#1478
- Related to apple/swift-protobuf#1480
- Resolves #196
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants