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

[SignalR] Add convenient POJOs. #774

Merged
merged 4 commits into from
Feb 16, 2022
Merged

[SignalR] Add convenient POJOs. #774

merged 4 commits into from
Feb 16, 2022

Conversation

Y-Sindo
Copy link
Member

@Y-Sindo Y-Sindo commented Jan 12, 2022

Issue describing the changes in this PR

resolves partially #771

Pull request checklist

  • My changes do not require documentation changes
    • Otherwise: Documentation issue linked to PR
  • My changes should not be added to the release notes for the next release
    • Otherwise: I've added my notes to release_notes.md
  • My changes do not need to be backported to a previous version
    • Otherwise: Backport tracked by issue/PR #issue_or_pr
  • I have added all required tests (Unit tests, E2E tests)
    I have tested it manually, but I don't know how to write unit tests or E2E tests for it.

@Y-Sindo Y-Sindo marked this pull request as ready for review January 12, 2022 14:48
@fabiocav fabiocav requested a review from kshyju January 12, 2022 21:13
/// <summary>
/// The type of a group action.
/// </summary>
[JsonConverter(typeof(JsonStringEnumConverter))]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Question: Wonder how it will work if customer chooses to override JSON serializer to be JSON.NET.

Copy link
Member Author

@Y-Sindo Y-Sindo Jan 14, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have tried to change the JSON serializer to Newtonsoft.JSON as the code below and it still works. Maybe the conversion for bindings is not controlled by users. I am not sure about this, maybe we need a dotnet worker expert to confirm this.

    class Program
    {
        static async Task Main(string[] args)
        {
            var host = new HostBuilder()
                .ConfigureFunctionsWorkerDefaults(o =>
                {
                    var settings = NewtonsoftJsonObjectSerializer.CreateJsonSerializerSettings();
                    settings.ContractResolver = new CamelCasePropertyNamesContractResolver();
                    settings.NullValueHandling = NullValueHandling.Ignore;
                    o.Serializer = new NewtonsoftJsonObjectSerializer(settings);
                })
                .Build();
            await host.RunAsync();

Copy link
Member Author

@Y-Sindo Y-Sindo Jan 14, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have found that NewtonsoftJsonObjectSerializer still works because Newtonsoft.Json has built-in support for enum->string.

Copy link
Member Author

@Y-Sindo Y-Sindo Jan 14, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there any way to avoid customers changing the serialization and deserialization of some field of my POJO? For example, I want to make the enum SignalRGroupAction serialized to string instead of number no matter how customers set their object serializer. But for the Arguments field in SignalRInvocationContext, SignalRMessageAction, I want to let the customers to control the serialization/deserialization.
@fabiocav

I think what we need can be concluded to have independent JSON serialization/deserialization for these POJO so that I can control which fields should be serialized/deserialized with WorkerOptions.Serializaer and which should not.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is not a blocker to merge this change. Something we should validate though.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To add context. No, today, the configured serializer will be used by default. We're adding features that will give binding authors better control of this, but in the meantime, I agree with @kshyju , we don't need to block, but it might be something worth documenting.


/// <summary>
/// The headers of request.
/// Headers with duplicated key will be joined by comma.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

will be joined by comma.

Or should we consider using <string,stringvalues> as the type?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it is a good idea, but deserialization is still a problem. If we customize the deserialization, how to avoid the customization changed by customers.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would be nice (not critical) to match how HttpRequestData exposes this information for consistency, but that uses HttpHeadersCollection

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

HttpHeadersCollection is in a .NET 5 project, and cannot be referenced by .NET Standard 2.0 project.

- Storage extensions moved to 5.0.0
- SignalR extensions added some convenient POJOs (PR #774)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should also consider adding sample usage of this types in the extension samples.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Currently the extension samples use package reference, so we cannot add new usages to the samples until they are released.


/// <summary>
/// The headers of request.
/// Headers with duplicated key will be joined by comma.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would be nice (not critical) to match how HttpRequestData exposes this information for consistency, but that uses HttpHeadersCollection

@Y-Sindo
Copy link
Member Author

Y-Sindo commented Jan 29, 2022

I think the PR is ready to be merged now. Thanks!

@kshyju
Copy link
Member

kshyju commented Feb 16, 2022

I think the PR is ready to be merged now. Thanks!

Feel free to merge.

@Y-Sindo
Copy link
Member Author

Y-Sindo commented Feb 16, 2022

I think the PR is ready to be merged now. Thanks!

Feel free to merge.

I don't have the write access to merge currently.

@kshyju
Copy link
Member

kshyju commented Feb 16, 2022

/check-enforcer evaluate

@kshyju kshyju merged commit acb1d28 into Azure:main Feb 16, 2022
@Y-Sindo Y-Sindo deleted the signalr branch February 18, 2022 07:19
@Y-Sindo Y-Sindo mentioned this pull request Jun 29, 2022
7 tasks
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.

3 participants