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:

• [x] Code compiles correctly
• [x] 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 https://github.com/dapr/dapr/issues/6324), the path is converted to v1.0/state/somekey, so it now returns a 404 because it can't match any route.

[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?

https://github.com/dapr/js-sdk/blob/b063e10e4b1a122fe7bc266d1681ca0727b6a522/src/actors/client/ActorClient/ActorClientHTTP.ts#L50

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?

https://github.com/dapr/js-sdk/blob/b063e10e4b1a122fe7bc266d1681ca0727b6a522/src/actors/client/ActorClient/ActorClientHTTP.ts#L50

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[bot]]

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

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

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

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: 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!