Skip to content

fix for optional parameters#108

Closed
HoMS1987 wants to merge 3 commits into
Astn:masterfrom
HoMS1987:master
Closed

fix for optional parameters#108
HoMS1987 wants to merge 3 commits into
Astn:masterfrom
HoMS1987:master

Conversation

@HoMS1987
Copy link
Copy Markdown

@HoMS1987 HoMS1987 commented Sep 2, 2019 •
edited
Loading

when only the second or third optional parameter is used during a JsonRequest, the JsonException "invalid parameters" was triggerd
now fixed => place of optional parameter in declaration of JsonRpcMethod doesn't matter anymore

@HoMS1987
fix for optional parameters coming in "JsonRpcMethod"
bfb832f 
when only the second or third optional parameter is used during a JsonRequest, the JsonException "invalid parameters" was triggerd
now fixed => place of optional parameter in declaration of JsonRpcMethod doesn't matter anymore
@HoMS1987
additional fixes after testing
cf4e479
@HoMS1987
fixed bug for matching JsonRpc parameters to method parameters
e315c66
@HoMS1987
Copy link
Copy Markdown
Author

HoMS1987 commented May 18, 2020

is this useful for the project?

@HoMS1987 HoMS1987 mentioned this pull request May 18, 2020
Closed
@Astn
Copy link
Copy Markdown
Owner

Astn commented May 18, 2020

Hi, @HoMS1987 This does look like it may be useful. How would I reproduce the issue? I need to have tests for what we are fixing here.

@Astn
Copy link
Copy Markdown
Owner

Astn commented May 18, 2020

@HoMS1987 There are some tests for optional parameters here: https://github.com/Astn/JSON-RPC.NET/blob/master/AustinHarris.JsonRpcTestN/Test.cs#L1074

Could you add one that demonstrates he issue?

@HoMS1987
Copy link
Copy Markdown
Author

HoMS1987 commented May 19, 2020 •
edited
Loading

The method where the error occured was similar to this:

    [JsonRpcMethod]
    private string TestDifferentOptionalParameters(string uid, string location = null, List<float> traces = null, List<float> wavelengths = null)
    {
        return "this is the requested measurement";
    }

The test cases would be like:

    [Test()]
    public void TestDifferentOptionalParametersNamedWorking()
    {
        string request = @"{method:'TestDifferentOptionalParameters',params:{location:'loc1', uid:'abc123', wavelengths: [0.0], traces: [0.0]},id:1";
        string expectedResult = "{\"jsonrpc\":\"2.0\",\"result\":\"this is the requested measurement\",\"id\":1}";
        var result = JsonRpcProcessor.Process(request);
        result.Wait();
        Assert.IsFalse(result.Result.Contains("error"));
        Assert.AreEqual(expectedResult, result.Result);
    }
    [Test()]
    public void TestDifferentOptionalParametersNamedNotWorking()
    {
        string request = @"{method:'TestDifferentOptionalParameters',params:{location:'loc1', uid:'abc123', wavelengths: [0.0]},id:1";
        string expectedResult = "{\"jsonrpc\":\"2.0\",\"result\":\"this is the requested measurement\",\"id\":1}";
        var result = JsonRpcProcessor.Process(request);
        result.Wait();
        Assert.IsFalse(result.Result.Contains("error"));
        Assert.AreEqual(expectedResult, result.Result);
    }

@HoMS1987
Copy link
Copy Markdown
Author

HoMS1987 commented May 19, 2020

The problem is, I have several optional datatypes. The params of the request don't have the same order like in the method. In my case it's not working when I get the first and third optional parameter, the second I don't get.

When I remember correct (the pull request is some months old^^) it's because in C# when you have a method with optional parameters, you can't just call it with the first and third optional parameter. It depends on the order of the implementation of the method. So if the json request only has the first and third optional parameter you automatically have to add the default value for the second optional parameter. and that's what I did in the pull request.

@Astn
Copy link
Copy Markdown
Owner

Astn commented May 21, 2020

Gotcha, I'll check it out. Thanks for the PR!

@Astn Astn mentioned this pull request May 22, 2020
Merged
@Astn
Copy link
Copy Markdown
Owner

Astn commented May 22, 2020

There were a few tests covering some edge cases that were not passing with this solution, so I had to take a slightly different approach to the problem to keep all the tests passing.

See PR #119

Again thanks for all your work on this issue!

@Astn Astn closed this May 22, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@HoMS1987 @Astn