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

Add support for JsonPropertyNameAttribute from System.Text.Json #925

Closed
juaal12 opened this issue Jun 8, 2023 · 10 comments
Closed

Add support for JsonPropertyNameAttribute from System.Text.Json #925

juaal12 opened this issue Jun 8, 2023 · 10 comments

Comments

@juaal12
Copy link

juaal12 commented Jun 8, 2023

Hi,

Is it planned to add support for System.Text.Json?

I have seen several closed issues regarding this topic (#766 and #305 for example).

In my case I have to use Newtonsoft just to apply a property renaming on my server call. I think we must support System.Text.Json since it is faster, lighter and for sure it will the be standard way for future applications since .NET 6 introduced it.

Thanks in advance!

@juaal12 juaal12 changed the title Add support for JsonPropertyNameAttribute on System.Text.Json Add support for JsonPropertyNameAttribute from System.Text.Json Jun 8, 2023
@AArnott
Copy link
Member

AArnott commented Jun 12, 2023

#908 in fact has already merged with System.Text.Json support coming in 2.16.

@AArnott AArnott closed this as not planned Won't fix, can't repro, duplicate, stale Jun 12, 2023
@taooceros
Copy link

Hi, when shall 2.16 be released?

@AArnott
Copy link
Member

AArnott commented Aug 10, 2023

@taooceros It was today.

@juaal12
Copy link
Author

juaal12 commented Sep 18, 2023

Good morning!

@AArnott

I think the last package release does not solve the support issue. With version 2.16.36 using JsonPropertyName is not behaving like JsonProperty provided by Newtonsoft.

[JsonProperty("clientId")]
public string ClientId { get; set; }

works fine since on generated json ClientId will be clientId. This is not happening when:

[JsonPropertyName("clientId")]
public string ClientId { get; set; }

since ClientId will still be ClientId.

Using System.Text.Json results in having a RemoteException by my side due to this behavior. Should we reopen the issue or should we create a new one?

@juaal12
Copy link
Author

juaal12 commented Sep 18, 2023

Hi,

You can notice it does not work fine just upgrading within the test application streamjson package and adding this lines in WebSocketClient Program.cs:

var stringResult = await jsonRpc.InvokeWithParameterObjectAsync<string>("TestMethod", 
                              new TestDto { TestA = "Client", TestB = "Secret" }, cancellationToken);
    public class TestDto
    {
        [JsonPropertyName("clientId")]
        public string TestA { get; set; }
        [JsonPropertyName("clientSecret")]
        public string TestB { get; set; }
    }

And then these others in JsonRpcServer.cs on in Web project:

        public string TestMethod(string clientId, string clientSecret)
        {
            return string.Concat(clientId, clientSecret);
        }

@AArnott
Copy link
Member

AArnott commented Sep 19, 2023

I'll take a look.

@AArnott AArnott reopened this Sep 19, 2023
@AArnott
Copy link
Member

AArnott commented Sep 21, 2023

It's funny, because we have a test that suggests the attribute works:

[Fact]
public void STJAttributesWinOverDataContractAttributesByDefault()
{
IJsonRpcMessageFactory messageFactory = this.Formatter;
JsonRpcRequest requestMessage = messageFactory.CreateRequestMessage();
requestMessage.Method = "test";
requestMessage.Arguments = new[] { new DCSClass { C = 1 } };
using Sequence<byte> sequence = new();
this.Formatter.Serialize(sequence, requestMessage);
using JsonDocument doc = JsonDocument.Parse(sequence);
this.Logger.WriteLine(doc.RootElement.ToString());
Assert.Equal(1, doc.RootElement.GetProperty("params")[0].GetProperty("B").GetInt32());
}

(look further down in the file to see use of the attribute).

We also have this test:

[Fact]
public async Task InvokeWithParameterObject_WithRenamingAttributes()
{
var param = new ParamsObjectWithCustomNames { TheArgument = "hello" };
string result = await this.clientRpc.InvokeWithParameterObjectAsync<string>(nameof(Server.ServerMethod), param, this.TimeoutToken);
Assert.Equal(param.TheArgument + "!", result);
}

Which demonstrates it works for custom named argument objects.

So I tried your repro. It behaves for me. Can you send me a repro project that I can execute?

@juaal12
Copy link
Author

juaal12 commented Sep 26, 2023

Sure!

I have forked your StreamJsonRpc.Sample repo and I have implemented the modifications in my previous message. Find here the repo: https://github.com/juaal12/StreamJsonRpc.Sample.

Thanks in advance!

@AArnott
Copy link
Member

AArnott commented Sep 28, 2023

Thanks, @juaal12. But your sample doesn't use the formatter for System.Text.Json at all. It's using Newtonsoft.Json. That would be why your attributes are being ignored.
If you want to use the System.Text.Json attributes, change your sample to use SystemTextJsonFormatter from this library.
Relevant docs here.

@juaal12
Copy link
Author

juaal12 commented Sep 29, 2023

Thanks, @juaal12. But your sample doesn't use the formatter for System.Text.Json at all. It's using Newtonsoft.Json. That would be why your attributes are being ignored. If you want to use the System.Text.Json attributes, change your sample to use SystemTextJsonFormatter from this library. Relevant docs here.

Hi @AArnott ,

I have updated the repository with your proposals in the last message. You can take a look if you want but by my side, after applying the changes, it works like a charm, so maybe this is not an issue anymore and we can close the thread.

Nevertheless, if you want to try it, my repo has been updated!

Thanks a lot and sorry for the inconvenience!

@AArnott AArnott closed this as not planned Won't fix, can't repro, duplicate, stale Sep 29, 2023
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

No branches or pull requests

3 participants