-
Notifications
You must be signed in to change notification settings - Fork 344
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
Bug/476 multiple methods per interface with JSON serialization doesn´t work #1343
Bug/476 multiple methods per interface with JSON serialization doesn´t work #1343
Conversation
I did found out what is wrong with the devcontainer setup :) Will fix both here. Of course the blind implementation was broken. Will be fixed in the next hour. |
f847e87
to
a13100b
Compare
@onionhammer This is now ready to review :) That was a fun research. I tried to don't break existing code paths as much as I could. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks good to me, although I have no authority here :)
.NET 7 fell out of support May 14 of this year and .NET 8 will do the same on November 10, 2024 with .NET 9 being introduced in the same time frame. That should be right around the 1.15 release, so I think there will be a good case to be made to drop .NET 6 and .NET 7 explicit support, target .NET 8 instead and include .NET 9 in the test suite for 1.16. |
Signed-off-by: paule96 <[email protected]>
Signed-off-by: paule96 <[email protected]>
…terface Signed-off-by: paule96 <[email protected]>
Now the devcontainer uses docker in docker, so you can reach the dapr setup after you did run dapr init. This will then only affect the dev container, without compromising the host of the devcontainer Signed-off-by: paule96 <[email protected]>
Signed-off-by: paule96 <[email protected]>
Signed-off-by: paule96 <[email protected]>
Signed-off-by: paule96 <[email protected]>
Signed-off-by: paule96 <[email protected]>
adb8471
to
7be9077
Compare
@WhitWaldo if I change the default version to dotnet 9.0 the new security vulnerability scanning of the dotnet CLI turns on implicitly and produces, of course, a lot of errors. I guess the upgrading of the default SDK needs to be delayed. maybe a separate issue is needed to clean up the solution of, the old tests, old implementation, and what else is connected to it. I guess the documentation needs to be updated too. So for now I would try to just update the default to dotnet 8.0 (support until November 10, 2026, it's LTS), and postpone the change to dotnet 9.0 to a later point in time. |
That's right - the plan is to put off changing the targeted releases until after the release of Dapr 1.15. There, we'll target .NET 6 and .NET 8 still, so no changes needed in the short term. But we'll put something prominent announcing EOL in the release notes so it's clear that in 1.16 we'll drop support for .NET 6 in favor of .NET 8 (and language version C# 12). An open question on that thread is whether we should go on targeting .NET 9 as well (though I'm also inclined to lean yes since there are frequently so many performance improvements in each). You're right, that'll require some significant doc updates in addition to the other projects, but I've got a draft PR that'll likely handle the latter as I'm gradually adding nullability annotations throughout the .NET SDK and there are several other PRs in place that'll shift up some of the project layouts as well that'll help. Things to tackle after the next release! |
@WhitWaldo do you know anybody I could ping to get a review? (maybe yourself) |
@paule96 I'm not a maintainer, so my check isn't worth a whole lot. I suspect this will get more attention as we edge closer to the 1.15 release and there's more of a general code freeze. If it hasn't been looked at closer to that timeline, I'll give it a look myself (for whatever it might be worth). |
…h_json_serialisation_doesntwork
@halspang & @philliphoff friendly reminder |
@paule96 I'm a maintainer now, so happy to give this a look! I'm not already super familiar with how Dapr handles serialization under the hood, so I'm learning as I read through your changes here. The approach already taken registers the interface and references it throughout using an int, why it does this, I'm not entirely sure, but I'd assume it's to avoid name conflicts and shorten the amount of data passed around. If that's at all accurate, I'm concerned that this approach, which binds the serialization to the name of the method runs a risk of being insufficiently specific (especially in light of #1350 and the opportunity for polymorphic bindings reflecting the same method names). Now, perhaps that's not a problem, as different interfaces should register different IDs and you're keying the serializers by both this interface and method. Perhaps you've considered this, but given your richer familiarity with the underlying problem space, does it make more sense to have a similar ID registry for the method names as we appear to have for the actor interfaces rather than the approach you've taken here? Second, could you add some unit tests that prove that this approach works sufficiently well for method overloads? |
…h_json_serialisation_doesntwork
…h_json_serialisation_doesntwork
…h_json_serialisation_doesntwork
…h_json_serialisation_doesntwork
@WhitWaldo sorry for the late reply. Was kinda busy these days. My first comment to overloads is: It's not supported. Their is an explicit exception for that in the file: dotnet-sdk/src/Dapr.Actors/Description/InterfaceDescription.cs Lines 135 to 151 in ee8be67
I will try to see what I can do to enable support for it. But I guess it will still work with names instead of Ids. The problematic part is, that we need to type match here. And because the transport types are json, it maybe becomes a challenge. (not sure yet) And I can understand that actors get an "id". Because their polymorphy is possible. By the language design of methods I'm not aware of any function that will provide polymophic behavior to methods. The only thing that I could see, if to trick around with inheritance, to like use the |
Maybe it resolves the need of chmod for it 🤷♀️ Signed-off-by: paule96 <[email protected]>
Signed-off-by: paule96 <[email protected]>
Signed-off-by: paule96 <[email protected]>
@WhitWaldo I now have created the basic changes for method overloading. I will not push it to this branch or PR. The problem is, that by the design of the actors part of dapr, it doesn't seem to be supported. For now I would put it on hold, but would like to know where we can find the framework independent documentation for dapr. Where you can see how dapr works outside of the SDK you use.
to:
or if it has parameters:
And I'm quite sure that this will be a breaking change, in special if you try to call an actor from example a python app. |
…h_json_serialisation_doesntwork
The actor runtime itself is in dapr/dapr, but I don't know that there's any specific documentation available that details any of it. As I understand it, it works similarly under the hood to Orleans (well, sort of) and Service Fabric Actors (closer) |
@WhitWaldo so can we skip the topic for now on this pullrequest and maybe open a new issue for method overloading? Because that was not scope of this PR |
…h_json_serialisation_doesntwork
@paule96 I agree method overloading isn't in scope, but I'm concerned about the opportunity for developers attempt to do so and then run into an odd runtime experience (as happens where with more than one method per interface). I'm going to review this again this week and see if we can't get it merged as-is. |
…h_json_serialisation_doesntwork
…h_json_serialisation_doesntwork
@WhitWaldo is their a chance that you can take a look to this :) |
@paule96 It's in the queue - trying to get to it before the 1.15 release. |
@paule96 If you could go through and fix the minor naming notes throughout (e.g. "dapr" -> "Dapr" and "github" -> "GitHub" on the exceptions that are noted, I'd be happy to merge it. |
Signed-off-by: paule96 <[email protected]>
…erface_with_json_serialisation_doesntwork' into bug/476_multiple_methods_per_interface_with_json_serialisation_doesntwork Signed-off-by: paule96 <[email protected]>
@WhitWaldo done :) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me!
"ghcr.io/devcontainers/features/docker-from-docker:1": { | ||
"version": "20.10" | ||
"ghcr.io/devcontainers/features/docker-in-docker": { | ||
"version": "latest" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ideally, this should be pinned to a particular version in case breaking changes are introduced later on.
|
||
if(_wrappedRequestMessageTypes.Count > 1){ | ||
throw new NotSupportedException("JSON serialisation should always provide the actor method (or nothing), that was called" + | ||
" to support (de)serialisation. This is a dapr sdk error, open an issue on github."); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"dapr" -> "Dapr"
"github" -> "GitHub"
I neglected to say it earlier - thank you very much for your contribution here @paule96 ! |
…t work (dapr#1343) * update devcontainer Signed-off-by: paule96 <[email protected]> * update test setup Signed-off-by: paule96 <[email protected]> * Now the json serialization should work with multiple methods in an interface Signed-off-by: paule96 <[email protected]> * fixed devcontainer to run actors Now the devcontainer uses docker in docker, so you can reach the dapr setup after you did run dapr init. This will then only affect the dev container, without compromising the host of the devcontainer Signed-off-by: paule96 <[email protected]> * fix bugs with the current implementation Signed-off-by: paule96 <[email protected]> * add a test that checks excatly the behavior Signed-off-by: paule96 <[email protected]> * fix devcontainer post creatd command Signed-off-by: paule96 <[email protected]> * change the default to dotnet 8.0 Signed-off-by: paule96 <[email protected]> * I don't know what is different but we commit. Maybe it resolves the need of chmod for it 🤷♀️ Signed-off-by: paule96 <[email protected]> * make it easier to see why the application of an E2E test couldn't start Signed-off-by: paule96 <[email protected]> * make the exception in E2E more percise Signed-off-by: paule96 <[email protected]> * fix exception message Signed-off-by: paule96 <[email protected]> --------- Signed-off-by: paule96 <[email protected]> Co-authored-by: Yaron Schneider <[email protected]> Co-authored-by: Whit Waldo <[email protected]> Signed-off-by: Divya Perumal <[email protected]>
…t work (dapr#1343) * update devcontainer Signed-off-by: paule96 <[email protected]> * update test setup Signed-off-by: paule96 <[email protected]> * Now the json serialization should work with multiple methods in an interface Signed-off-by: paule96 <[email protected]> * fixed devcontainer to run actors Now the devcontainer uses docker in docker, so you can reach the dapr setup after you did run dapr init. This will then only affect the dev container, without compromising the host of the devcontainer Signed-off-by: paule96 <[email protected]> * fix bugs with the current implementation Signed-off-by: paule96 <[email protected]> * add a test that checks excatly the behavior Signed-off-by: paule96 <[email protected]> * fix devcontainer post creatd command Signed-off-by: paule96 <[email protected]> * change the default to dotnet 8.0 Signed-off-by: paule96 <[email protected]> * I don't know what is different but we commit. Maybe it resolves the need of chmod for it 🤷♀️ Signed-off-by: paule96 <[email protected]> * make it easier to see why the application of an E2E test couldn't start Signed-off-by: paule96 <[email protected]> * make the exception in E2E more percise Signed-off-by: paule96 <[email protected]> * fix exception message Signed-off-by: paule96 <[email protected]> --------- Signed-off-by: paule96 <[email protected]> Co-authored-by: Yaron Schneider <[email protected]> Co-authored-by: Whit Waldo <[email protected]> Signed-off-by: Siri Varma Vegiraju <[email protected]>
Description
Now the Actor manager provides the
MethodName
for the serializationActorMessageSerializersManager
.This makes it now possible for the JSON serialization to support Actor interfaces with more than one method defined in it.
As a side effect, this PR solves #1342 too.
Changes made in the dev container:
C# DevKit
as an additional extension. This can be a breaking change for people that don't have a visual studio license and want to run the dev container on their local host. Users that run the dev container on GitHub get a license automatically injected.localinit.sh
script before running itChanges made to the E2E Tests
Issue reference
It's part of the issue #476 starting with this comment.
It fixes also #1342
Checklist
Please make sure you've completed the relevant tasks for this PR, out of the following list:
cc @onionhammer as original contributor :)