feat: make it possible to override `method name` in subscriptions #568

niklasad1 · 2021-11-17T18:33:14Z

Fixes paritytech/substrate#8783 (comment) to be compatible with current the behavior on substrate master to make it possible to configure custom method name for subscriptions (i.e, not the same as subscription method name) to avoid breakage for libraries that can't distinguish between ordinary notifications and subscription notifications

Example:

/// Use `override` as method name in the subscription notif
#[subscription(name = "subscribe_method" => "override", item = u32)]
fn sub_with_override(&self) -> RpcResult<()>;


/// Use default `subscription name` as method name in the subscription notif
#[subscription(name = "subscribeHello", item = u32)]
fn sub_with_override(&self) -> RpcResult<()>;

dvdplm · 2021-11-18T09:21:16Z

Thoughts about this such as naming?!

I absolutely hate it. 😆

I would like to not have to support this, or at the very least understand why we need it.

…ethod-name

maciejhirsz

Some open questions, but looks good to me otherwise!

maciejhirsz · 2021-11-18T15:23:35Z

utils/src/server/rpc_module.rs

 }

 impl<Context> RpcModule<Context> {
 	/// Create a new module with a given shared `Context`.
 	pub fn new(ctx: Context) -> Self {
-		Self { ctx: Arc::new(ctx), methods: Default::default() }
+		Self { ctx: Arc::new(ctx), methods: Default::default(), notif_overrides: SubscriptionNotifOverride::default() }


This doesn't really prevent us from having the same notification name in two different modules (the set is local to the instance and is thrown away the moment you turn it into Methods).

If we can't easily guarantee that notification names are unique, and there isn't really much of a point in checking that they are in the server (notifications will be sent regardless, and we rely on sub id more than name on the client anyway), I think it might actually be better to drop the check altogether.

ty, can't we handle it in RpcModule::merge?

You can, but you'd have to move the hashset inside Methods because that's what merge operates on.

let's keep it simple I removed all of this but kept the check in the proc macros for now, ok?

Good pt @maciejhirsz. I'm ok making this a "sharp" feature, but we need to document it properly. "NOTE: the author is expected to ensure the uniqueness of the override across all RpcModules used on the server." or some such.

Note, we have a check for this in the proc macros where each trait ensures uniqueness for each name.

Do you want me to remove it be to somewhat consistent?

utils/src/server/rpc_module.rs

proc-macros/src/rpc_macro.rs

…nto na-override-notif-method-name

dvdplm

Sorry for being a trouble maker wrt naming; I'm concerned about mixing up the concept of "notification" with the spec's definition.
We need to update the macro docs with a clear explanation of what this change is and when it's useful (stressing it should never be used for new code; people should use the subscription ID to track things).

proc-macros/src/render_server.rs

proc-macros/src/rpc_macro.rs

proc-macros/tests/ui/correct/basic.rs

utils/src/server/rpc_module.rs

TarikGul · 2021-11-19T13:37:43Z

tests/tests/proc_macros.rs

@@ -312,7 +312,7 @@ async fn multiple_blocking_calls_overlap() {

 #[tokio::test]
 async fn subscriptions_do_not_work_for_http_servers() {
-	let htserver = HttpServerBuilder::default().build("127.0.0.1:0".parse().unwrap()).unwrap();
+	let htserver = HttpServerBuilder::default().build("127.0.0.1:0").unwrap();


TarikGul

lgtm, fwiw I agree with David that some clearer naming could be a +1.

Co-authored-by: David <[email protected]>

maciejhirsz · 2021-11-19T13:48:39Z

@dvdplm

I'm concerned about mixing up the concept of "notification" with the spec's definition.

So I did re-read things to make sure I have it right. There is nothing wrong with us using the term "Notification", and the spec explicitly allows / remains agnostic about server acting as a client:

The Client is defined as the origin of Request objects and the handler of Response objects.
The Server is defined as the origin of Response objects and the handler of Request objects.

One implementation of this specification could easily fill both of those roles, even at the same time, to other different clients or the same client. This specification does not address that layer of complexity.

We are using JSON-RPC notifications in a very specific fashion, but I don't see how that stops them from being notifications. TCP packets don't stop being TCP packets if they carry HTTP paylods. Everything we do with subscriptions is fully compliant with the JSON-RPC spec, we don't change anything about the spec, nor do we add anything to the spec, subscriptions are just a meta protocol bolted on top of it.

Co-authored-by: David <[email protected]>

niklasad1 · 2021-11-19T13:57:19Z

lgtm, fwiw I agree with David that some clearer naming could be a +1.

Technically the naming is accurate because subscriptions is a notification according the the spec which has a special field subscription passed in the params where the subscription ID goes but subscription payload works too if that's more understandable....

Good to get your review still to get some eyes outside the jsonrpc bubble :)

TarikGul · 2021-11-19T14:15:00Z

Technically the naming is accurate because subscriptions is a notification according the the spec which has a special field subscription passed in the params where the subscription ID goes but subscription payload works too if that's more understandable....

@niklasad1 Just read @maciejhirsz comments as well. Totally understand where the convention is coming from. Happy to see it kept as is as well. With a third glance over I can see it makes sense to have notif. :)

dvdplm · 2021-11-19T17:48:29Z

@maciejhirsz You are right and I am wrong; the spec is indeed agnostic just the way you quote it. My concern was we'd introduce a naming convention that conflicts with a specific meaning defined in the spec. Instead there is no such conflict.

I'll go over my naming suggestions again and see if I know feel differently. Ty!

proc-macros/src/render_server.rs

proc-macros/src/rpc_macro.rs

dvdplm · 2021-11-19T17:53:44Z

proc-macros/tests/ui/correct/basic.rs

@@ -31,6 +31,11 @@ pub trait Rpc {

 	#[subscription(name = "echo", aliases = ["ECHO"], item = u32, unsubscribe_aliases = ["NotInterested", "listenNoMore"])]
 	fn sub_with_params(&self, val: u32) -> RpcResult<()>;
+
+	// This will send data to subscribers with the `method` field in the JSON payload set to `foo_subscribe_override`
+	// because it's in the `foo` namespace.


oh that's a good point, let's not forget to include that in the docs for the macro.

utils/src/server/rpc_module.rs

Co-authored-by: David <[email protected]>

feat: override method subscription notif

df60861

niklasad1 changed the title ~~feat: override notif method name~~ feat: override method name in subscription notif Nov 17, 2021

maciejhirsz and others added 5 commits November 18, 2021 13:40

Arrow syntax for overwrites (#569)

d6eb4ea

Merge remote-tracking branch 'origin/master' into na-override-notif-m…

86ea9db

…ethod-name

check that unique notifs are used

84aad42

check that custom sub name is unique

eea1b2b

cargo fmt

e72a33a

maciejhirsz approved these changes Nov 18, 2021

View reviewed changes

address grumbles

673699a

niklasad1 commented Nov 18, 2021

View reviewed changes

proc-macros/src/rpc_macro.rs Outdated Show resolved Hide resolved

niklasad1 added 3 commits November 18, 2021 17:06

Update proc-macros/src/rpc_macro.rs

2913c0b

commit added tests

37f77d1

Merge remote-tracking branch 'origin/na-override-notif-method-name' i…

70477f9

…nto na-override-notif-method-name

niklasad1 changed the title ~~feat: override method name in subscription notif~~ feat: make it possible to override method name in subscriptions Nov 18, 2021

niklasad1 requested review from dvdplm and TarikGul November 18, 2021 22:09

dvdplm reviewed Nov 19, 2021

View reviewed changes

TarikGul reviewed Nov 19, 2021

View reviewed changes

TarikGul approved these changes Nov 19, 2021

View reviewed changes

Update proc-macros/src/render_server.rs

66972ef

Co-authored-by: David <[email protected]>

niklasad1 and others added 4 commits November 19, 2021 14:53

Update proc-macros/src/render_server.rs

5067f0d

Co-authored-by: David <[email protected]>

Update proc-macros/src/rpc_macro.rs

6886c2b

Co-authored-by: David <[email protected]>

Update proc-macros/src/rpc_macro.rs

1eb0a80

Co-authored-by: David <[email protected]>

Update utils/src/server/rpc_module.rs

2be32e9

Co-authored-by: David <[email protected]>

niklasad1 added 2 commits November 19, 2021 15:37

grumbles

b67be8f

fix long lines

3e8a18e

dvdplm approved these changes Nov 19, 2021

View reviewed changes

niklasad1 and others added 6 commits November 19, 2021 19:06

Update utils/src/server/rpc_module.rs

48ebf92

Co-authored-by: David <[email protected]>

Update utils/src/server/rpc_module.rs

b01fbe8

Co-authored-by: David <[email protected]>

Update proc-macros/src/rpc_macro.rs

a1a9577

Co-authored-by: David <[email protected]>

Update proc-macros/src/render_server.rs

86b06ed

Co-authored-by: David <[email protected]>

Update proc-macros/src/render_server.rs

b76efe8

Co-authored-by: David <[email protected]>

more grumbles

b20a0c3

niklasad1 merged commit 9c6fd4b into master Nov 19, 2021

niklasad1 deleted the na-override-notif-method-name branch November 19, 2021 18:30

dvdplm mentioned this pull request Nov 22, 2021

jsonrpsee integration paritytech/substrate#8783

Merged

8 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: make it possible to override `method name` in subscriptions #568

feat: make it possible to override `method name` in subscriptions #568

niklasad1 commented Nov 17, 2021 •

edited

Loading

dvdplm commented Nov 18, 2021

maciejhirsz left a comment

maciejhirsz Nov 18, 2021 •

edited

Loading

niklasad1 Nov 18, 2021

maciejhirsz Nov 18, 2021

niklasad1 Nov 18, 2021

dvdplm Nov 19, 2021

niklasad1 Nov 19, 2021 •

edited

Loading

dvdplm left a comment

TarikGul Nov 19, 2021

TarikGul left a comment

maciejhirsz commented Nov 19, 2021 •

edited

Loading

niklasad1 commented Nov 19, 2021 •

edited

Loading

TarikGul commented Nov 19, 2021

dvdplm commented Nov 19, 2021

dvdplm Nov 19, 2021

feat: make it possible to override method name in subscriptions #568

feat: make it possible to override method name in subscriptions #568

Conversation

niklasad1 commented Nov 17, 2021 • edited Loading

dvdplm commented Nov 18, 2021

maciejhirsz left a comment

Choose a reason for hiding this comment

maciejhirsz Nov 18, 2021 • edited Loading

Choose a reason for hiding this comment

niklasad1 Nov 18, 2021

Choose a reason for hiding this comment

maciejhirsz Nov 18, 2021

Choose a reason for hiding this comment

niklasad1 Nov 18, 2021

Choose a reason for hiding this comment

dvdplm Nov 19, 2021

Choose a reason for hiding this comment

niklasad1 Nov 19, 2021 • edited Loading

Choose a reason for hiding this comment

dvdplm left a comment

Choose a reason for hiding this comment

TarikGul Nov 19, 2021

Choose a reason for hiding this comment

TarikGul left a comment

Choose a reason for hiding this comment

maciejhirsz commented Nov 19, 2021 • edited Loading

niklasad1 commented Nov 19, 2021 • edited Loading

TarikGul commented Nov 19, 2021

dvdplm commented Nov 19, 2021

dvdplm Nov 19, 2021

Choose a reason for hiding this comment

feat: make it possible to override `method name` in subscriptions #568

feat: make it possible to override `method name` in subscriptions #568

niklasad1 commented Nov 17, 2021 •

edited

Loading

maciejhirsz Nov 18, 2021 •

edited

Loading

niklasad1 Nov 19, 2021 •

edited

Loading

maciejhirsz commented Nov 19, 2021 •

edited

Loading

niklasad1 commented Nov 19, 2021 •

edited

Loading