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

Apply string casing to method params while deriving proxies and services #30

Open
wants to merge 1 commit into
base: 1.x
Choose a base branch
from

Conversation

Eligioo
Copy link
Member

@Eligioo Eligioo commented Dec 13, 2024

When providing RPC method parameters as a client via named parameters {"a": 1, "b": 2}, they had to be snake_cased since no string case renaming was applied to them inside the deriving macros. This PR changes this behaviour such that it adheres to the rename_all attribute that can be provided to the Proxy and Service macros.

Old input with service/proxy(rename_all = "camelCase") applied:

{
	"jsonrpc": "2.0",
	"method": "getElectionBlockAfter",
	"params": {
		"block_number": 33
	},
	"id": 1
}	

New input with service/proxy(rename_all = "camelCase") applied:

{
	"jsonrpc": "2.0",
	"method": "getElectionBlockAfter",
	"params": {
		"blockNumber": 33
	},
	"id": 1
}	

@Eligioo Eligioo requested review from sisou, jsdanielh and hrxi December 13, 2024 13:42
@sisou
Copy link
Member

sisou commented Dec 13, 2024

No idea how this magic works, so I'll have to try it out, which is a bit more work to do. Next week probably.

@blouflashdb
Copy link

Is this a breaking change for people that still use old casing?

@Eligioo
Copy link
Member Author

Eligioo commented Dec 16, 2024

Is this a breaking change for people that still use old casing?

@blouflashdb I would consider this a local breaking change yes and not protocol breaking one. If a client uses snake casing, it has to be updated. However I'm assuming that most users are using positional parameters thus not affect many.

@blouflashdb
Copy link

blouflashdb commented Dec 16, 2024

Please make it backwards compatible. Deprecate the old casing and remove it completely in a major version and add it to the breaking changes section in the upgrade notes.

I believe the next version will be a patch version? You can not expect from users to change their rpc usage in a patch version...

@Eligioo Eligioo force-pushed the stefan/params-allow-rename branch from 668bcfa to 4762c06 Compare December 16, 2024 08:13
@Eligioo Eligioo force-pushed the stefan/params-allow-rename branch from 4762c06 to 633794f Compare December 16, 2024 08:18
@Eligioo
Copy link
Member Author

Eligioo commented Dec 16, 2024

jsonrpc eventually ends up as a dependency used by the nimiq-client which follows its own SemVer. It's nowhere stated that this change is going to be included in the next release of the nimiq-client.

@Eligioo Eligioo changed the base branch from master to 1.x December 16, 2024 09:19
@hrxi
Copy link
Contributor

hrxi commented Dec 16, 2024

Would probably be good to have a transition period where we accept both to be backward compatible.

@sisou
Copy link
Member

sisou commented Dec 16, 2024

This jsonrpc crate is published itself to crates.io. So it has it's own semver and @blouflashdb is right that this is a breaking change if the old style is not accepted anymore, requiring a new major version of this crate.

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.

4 participants