Add Actor ID validation

[heunghingwan]

Description

Throw when actor id contain '/'

Issue reference

issue this PR will close: #537

Checklist

Please make sure you've completed the relevant tasks for this PR, out of the following list:

• Code compiles correctly
• Created/updated tests
• Extended the documentation

[heunghingwan]

Add empty checking after some reading in dapr/dapr#6461

Take for example the route v1.0/state/{storeName}/{key}. Before, passing an empty {storeName}, such as v1.0/state//somekey would have still invoked the handler, which would have received an empty "storeName" (and likely returned 400 - bad request). Because the router now normalizes paths and removes double slashes (see dapr/dapr#6324), the path is converted to v1.0/state/somekey, so it now returns a 404 because it can't match any route.

[XavierGeerinck]

Thank you for adding a test!!! Almost there, just a small change :)

[XavierGeerinck]

Can we still change if (id === "") throw new Error("ActorId cannot be empty"); ?

[heunghingwan]

Can we still change if (id === "") throw new Error("ActorId cannot be empty"); ?

Yes, it will be more versatile when importing in a javascript-only application

[XavierGeerinck]

Can we still change if (id === "") throw new Error("ActorId cannot be empty"); ?

Yes, it will be more versatile when importing in a javascript-only application

Perfect! But please use the multi line if instead of single one

if (!id) {
...
}

[shubham1172]

@heunghingwan were you able to test your scenario from the issue with this PR?

js-sdk/src/actors/client/ActorClient/ActorClientHTTP.ts

Line 50 in b063e10

const result = await this.client.execute(`/actors/${actorType}/${actorId.getId()}/state/${key}`);

Since we are decoding back in getId, it should still not work since the ID will contain special characters again.

[heunghingwan]

@heunghingwan were you able to test your scenario from the issue with this PR?

js-sdk/src/actors/client/ActorClient/ActorClientHTTP.ts

Line 50 in b063e10

const result = await this.client.execute(`/actors/${actorType}/${actorId.getId()}/state/${key}`);

Since we are decoding back in getId, it should still not work since the ID will contain special characters again.

You are right, maybe add a getURISafeId function, or encoded in ActorClientHTTP.ts, to provide unencoded id for grpc use?

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (b063e10) 100.00% compared to head (59379c8) 100.00%.
Report is 3 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff            @@
##              main      #539   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files            1         1           
  Lines            6         6           
  Branches         1         1           
=========================================
  Hits             6         6           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[shubham1172]

@XavierGeerinck one issue I see with this change is, we are introducing a parity between HTTP and gRPC actor clients. HTTP will use a sanitized actor ID, and gRPC won't. This means there will be interop issues b/w the two protocols.

I would propose to modify gRPC's behavior too and keep it consistent. Thoughts?

[heunghingwan]

@XavierGeerinck one issue I see with this change is, we are introducing a parity between HTTP and gRPC actor clients. HTTP will use a sanitized actor ID, and gRPC won't. This means there will be interop issues b/w the two protocols.

I would propose to modify gRPC's behavior too and keep it consistent. Thoughts?

It should not behave differently, actor ID in gRPC is transmitted as-is, actor ID in HTTP should be decoded in core

[shubham1172]

@heunghingwan I see, it should be fine in that case, thanks for your contribution!