CoreFx #36125 (Try)Get for byte sbyte short ushort #38469

WinCPP · 2019-06-11T21:49:27Z

Addresses (Try)Get APIs for byte, sbyte, short and ushort for #36125

src/System.Text.Json/src/System/Text/Json/Reader/Utf8JsonReader.TryGet.cs

stephentoub · 2019-06-12T20:01:23Z

src/System.Text.Json/src/Resources/Strings.resx

+  </data>
+  <data name="FormatUInt16" xml:space="preserve">
+    <value>The JSON value is either of incorrect numeric format, or too large or too small for a UInt16.</value>
+  </data>


Nit: grammatically, these should be reworded slightly, e.g.

The JSON value either uses an incorrect numeric format or is too large or too small for a Byte.

Also, would it make sense to include in the error message the value that couldn't be parsed?

Nit: grammatically, these should be reworded slightly, e.g.

Also, would it make sense to include in the error message the value that couldn't be parsed?

@stephentoub does this sound ok? Will make changes in all the places accordingly.

The JSON value could not be parsed as a number, uses incorrect numeric format or is too large or too small for a Byte.

Functionally speaking, "uses an incorrect numeric format" is redundant with "could not be parsed as a number".

sbyte.Parse's failures are "Input string was not in a correct format." or "Value was either too large or too small for a signed byte."

Putting them together into one, I'd go with

The JSON value either was not in a supported format, or is out of bounds for a signed byte.

("out of bounds" to remove the second "or")

It looks like the primitive Parse methods use the type name (e.g. Int32) except for byte/sbyte, where it's "unsigned byte" (lowercase) and "signed byte" (lowercase). https://github.com/dotnet/coreclr/blob/29810a78e5b93d8da9fb921d096226d249fc75a5/src/System.Private.CoreLib/Resources/Strings.resx#L3130-L3162

The JSON value either was not in a supported format, or is out of bounds for a signed byte.

Should the tense be same in both clauses? We have was and is here. Intrigued, I searched and came across this,
https://grammar.collinsdictionary.com/easy-learning/joining-clauses

<quote> When it is used in this way either must come in one of these places: - before the subject in the first clause of the group. - in front of the main verb and after any auxiliary verb. </quote>

Looks like either needs to be before the subject in this case? E.g.,

Either the JSON value is not in a supported format, or is out of bounds for a signed byte.

Either the JSON value is not in a supported format, or is out of bounds for a signed byte.

That exception message looks good to me.

I suggest to include the invalid value. Can be pretty helpful for offline analysis. Quite often for example in random unit test failures in corefx we end up going in to add more logging of the actual value. Up to you

I suggest to include the invalid value.

Do you mean the actual JSON string? I think transcoding the ValueSpan/ValueSequence back to UTF-16 string and appending that to the exception message might be useful.

string json = "[12345, 1]"; byte[] utf8 = Encoding.UTF8.GetBytes(json); var reader = new Utf8JsonReader(utf8, ...); reader.Read(); // Start Array reader.Read(); // Number // This method call will throw with the message: // "Either the JSON value is not in a supported format, or is out of bounds for a signed byte." // What should we append here? Re-parse "12345" as a larger numeric type and append the value to the exception message? // OR, take the raw utf-8 bytes that represent 12345, transcode it back to UTF-16 string, and append that to the exception message? reader.GetByte();

@danmosemsft @ahsonkhan given the 6/26 deadline and that we haven't been including the actual JSON fragement into the error message until now, I am inclined to change the error message to the Either the JSON value is... string discussed above. For more changes perhaps we can have another issue?

That sounds good to me.

src/System.Text.Json/src/System/Text/Json/Reader/Utf8JsonReader.TryGet.cs

WinCPP · 2019-06-15T14:39:06Z

@ahsonkhan I have added equivalent APIs in JsonElement.

bartonjs · 2019-06-17T16:19:22Z

src/System.Text.Json/ref/System.Text.Json.cs

        public bool TryGetUInt32(out uint value) { throw null; }
        [System.CLSCompliantAttribute(false)]
        public bool TryGetUInt64(out ulong value) { throw null; }
        public bool TrySkip() { throw null; }
        public bool ValueTextEquals(System.ReadOnlySpan<byte> utf8Text) { throw null; }
        public bool ValueTextEquals(System.ReadOnlySpan<char> text) { throw null; }
-        public bool ValueTextEquals(string text) { throw null; }
+        public bool ValueTextEquals(string utf8Text) { throw null; }


This name change seems bad. Presumably the ref got manually corrected at some point and the src is out of date. (Or the src got changed after the last ref generation)

The correct name here is text, as System.String isn't UTF-8.

The discrepancy of parameter name between src and ref has been since introduction of the new API. Reference: f09135a

I will revert here and also change the parameter name in the API so that ref code auto-generated in future won't have this issue.

The same issue came up here: #38409 (comment)

I will revert here and also change the parameter name in the API so that ref code auto-generated in future won't have this issue.

That would be awesome! I was just about to submit a PR for this change, but I will wait for you to update it instead. Thanks.

bartonjs · 2019-06-17T16:36:12Z

src/System.Text.Json/tests/JsonDocumentTests.cs

+
+                Assert.Throws<InvalidOperationException>(() => root.GetString());
+                const string ThrowsAnyway = "throws-anyway";
+                Assert.Throws<InvalidOperationException>(() => root.ValueEquals(ThrowsAnyway));


There are a number of tests in this file that check for the InvalidOperationException on GetInt32(); the new one-and-two-byte-integer methods should be added to those tests as well.

Adding the same! Thanks!

ahsonkhan · 2019-06-18T03:35:25Z

src/System.Text.Json/ref/System.Text.Json.cs

@@ -148,8 +160,8 @@ public abstract partial class JsonNamingPolicy
    public partial struct JsonReaderOptions
    {
        private int _dummyPrimitive;
-        public bool AllowTrailingCommas { get { throw null; } set { } }
-        public System.Text.Json.JsonCommentHandling CommentHandling { get { throw null; } set { } }
+        public bool AllowTrailingCommas { readonly get { throw null; } set { } }


If you pull from head of master into your branch, these changes will go away.

src/System.Text.Json/src/System/Text/Json/Document/JsonElement.cs

src/System.Text.Json/ref/System.Text.Json.cs

ahsonkhan · 2019-06-18T03:39:57Z

src/System.Text.Json/ref/System.Text.Json.cs

        public float GetSingle() { throw null; }
        public string GetString() { throw null; }
        [System.CLSCompliantAttribute(false)]
+        public uint GetUInt16() { throw null; }


This should be ushort

Suggested change

public uint GetUInt16() { throw null; }

public ushort GetUInt16() { throw null; }

src/System.Text.Json/src/System/Text/Json/Document/JsonElement.cs

src/System.Text.Json/src/System/Text/Json/Reader/Utf8JsonReader.TryGet.cs

src/System.Text.Json/tests/JsonNumberTestData.cs

ahsonkhan · 2019-06-18T03:57:24Z

src/System.Text.Json/tests/JsonNumberTestData.cs

+            byte[] b16 = new byte[2];
+            for (int i = 0; i < numberOfItems; i++)
+            {
+                Shorts.Add(BitConverter.ToInt16(b16));


This isn't really adding new short values (it keeps adding 0 to the list). Same with ushort test cases below.

Missed random.NextBytes. Added.

The BitConverter.ToInt16 is implicitly casting the byte[] to a ReadOnlySpan<byte> and that API isn't available on netfx. Try using the other overload that takes byte[] and index.
https://apisof.net/catalog/System.BitConverter.ToInt16(Byte(),Int32)

Yup that whole test case is broken as you pointed out (it is putting in zeros). In few moments I am uploading a new patch which has all these fixes...

ahsonkhan · 2019-06-18T04:01:37Z

src/System.Text.Json/tests/Utf8JsonReaderTests.cs

            json = default;
            JsonTestHelper.AssertThrows<InvalidOperationException>(json, (jsonReader) => jsonReader.TryGetSingle(out _));
            JsonTestHelper.AssertThrows<InvalidOperationException>(json, (jsonReader) => jsonReader.GetSingle());

+            json = default;
+            JsonTestHelper.AssertThrows<InvalidOperationException>(json, (jsonReader) => jsonReader.TryGetUInt16(out _));


These new test additions look good. Can you please share the code coverage numbers for the new APIs being added?

danmoseley · 2019-06-22T15:40:32Z

This needs to be committed by noon 6/26 to make 3.0.

ahsonkhan · 2019-06-22T21:34:37Z

@WinCPP, let me know if you need any help or another review. I think just a few pieces of feedback are left to resolve (+ fixing the merge conflict).

WinCPP · 2019-06-22T21:38:42Z

@ahsonkhan I am not able to run tests after doing rebase. I get following errors. Did a clean build, but still same issues. Has something changed...? Please help.

  A fatal error was encountered. The library 'hostpolicy.dll' required to execute the application was not found in 'D:\WinCPP\corefx\artifacts\bin\System.Text.Json.Tes
  ts\netcoreapp-Debug\'.
  Failed to run as a self-contained app. If this should be a framework-dependent app, specify the appropriate framework in D:\WinCPP\corefx\artifacts\bin\System.Text.J
  son.Tests\netcoreapp-Debug\xunit.console.runtimeconfig.json.
  ----- end Sun 06/23/2019  2:10:22.61 ----- exit code -2147450749 ----------------------------------------------------------```

ahsonkhan · 2019-06-22T21:57:02Z

Has something changed...?

Not that I am aware off. I just grabbed master, and was able to build/run the S.T.Json.Tests myself.

Can you try building corefx/master locally and see if you are able to run the S.T.Json.Tests?

If that doesn't work, try git clean -fdx from root and rebuild (make sure any un-staged changes are staged/committed first). Make sure that your corefx/.dotnet folder gets deleted before trying to build again. Sometimes, if the repo/sdk gets in a strange state locally, it could result in that error.

I am not able to run tests after doing rebase. I get following errors. Did a clean build, but still same issues.

What did you do for a clean build and what command did you use to run the tests?

E:\GitHub\Fork\corefx> git clean -fdx
E:\GitHub\Fork\corefx> build.cmd
E:\GitHub\Fork\corefx\src\System.Text.Json\tests> dotnet msbuild /t:BuildAndTest

bartonjs · 2019-06-22T22:02:23Z

In a different terminal window:

cd artifacts\bin
dir /s xunit.console.runtimeconfig.json
(find a path to one that's around 149 bytes, the one under System.Text.Json.Tests is probably less than 30 bytes) (there really are only two sizes, so find "a" good one)
copy /y path\to\correct\xunit.console.runtimeconfig.json System.Text.Json.Tests\netcoreapp-Windows_NT\

In the first window:

msbuild /t:BuildAndTest /p:GenerateRuntimeConfigurationFiles=false
If it still fails, copy the file one more time, and now this command should succeed on a retry.

I don't really know why this is needed, but I got bit by it, too, and that was a viable workaround for me (different project, but same symptom)

WinCPP · 2019-06-23T11:14:06Z

@ahsonkhan

E:\GitHub\Fork\corefx> git clean -fdx
E:\GitHub\Fork\corefx> build.cmd
E:\GitHub\Fork\corefx\src\System.Text.Json\tests> dotnet msbuild /t:BuildAndTest

Yup had tried git clean -fdx but still it didn't work.

@bartonjs

msbuild /t:BuildAndTest /p:GenerateRuntimeConfigurationFiles=false

If it still fails, copy the file one more time, and now this command should succeed on a retry.

Thanks for inputs I could proceed but only till some extent. Now the test cases are failing for something not related to my code change, errors as below. Just to re-confirm I switched to master branch, did git clean -fdx then did build.cmd -clean followed by normal build and repeated the steps that you mentioned above and still I same errors.

  System.Text.Json.Tests -> D:\WinCPP\corefx\artifacts\bin\System.Text.Json.Tests\netcoreapp-Debug\System.Text.Json.Tests.dll
  ----- start Sun 06/23/2019 16:31:57.84 ===============  To repro directly: =====================================================
  pushd D:\WinCPP\corefx\artifacts\bin\System.Text.Json.Tests\netcoreapp-Debug\
  "D:\WinCPP\corefx\artifacts\bin\testhost\netcoreapp-Windows_NT-Debug-x64\dotnet.exe" xunit.console.dll System.Text.Json.Tests.dll -xml testResults.xml -nologo -notrait category=nonnetcoreapptests -notrait category=nonwindowstests -notrait category=OuterLoop -notrait category=failing
  popd
  ===========================================================================================================
    Discovering: System.Text.Json.Tests (method display = ClassAndMethod, method display options = None)
    Discovered:  System.Text.Json.Tests (found 1004 of 1030 test cases)
    Starting:    System.Text.Json.Tests (parallel test collections = on, max threads = 8)
      System.Text.Json.Serialization.Tests.CacheTests.PropertyCacheWithMinInputsFirst [FAIL]
        System.Text.Json.JsonException : The JSON value could not be converted to System.DateTime. Path: $.MySimpleTestStruct.MyDateTime | LineNumber: 0 | BytePositionInLine: 896.
        ---- System.FormatException : The JSON value is not in a supported DateTime format.

One observations if it helps tell more about my system: On my system, for System.Text.Json.* projects, netcoreapp-debug folder is generated and not netcoreapp-Windows_NT-Debug which is generated for say, System.Text.Encoding projects. Could this be cause of at least the 'DateTime' related test failures (seen also on master).

ahsonkhan · 2019-06-23T18:39:00Z

Now the test cases are failing for something not related to my code change, errors as below.

Are many tests failing or just the PropertyCacheWithMinInputsFirst? If that's the only one that's failing, can you please skip the failing test (use the Skip attribute, or the ActiveIssue attribute, or just comment it out) to unblock yourself and file an issue for it? Are you on a windows machine?

That particular test is trying to make sure the options cache is working as expected so there might be a bug that only reproduces intermittently. If it is consistently failing, can you please share the whole call stack as well, because I wasn't able to repro this issue (and it isn't happening on CI either).

cc @steveharter

I can try checking out your branch to see, is this it: https://github.com/WinCPP/corefx/tree/newtrygets?

WinCPP · 2019-06-23T19:45:13Z

Are many tests failing or just the PropertyCacheWithMinInputsFirst? If that's the only one that's failing, can you please skip the failing test (use the Skip attribute, or the ActiveIssue attribute, or just comment it out) to unblock yourself and file an issue for it? Are you on a windows machine?

Many test cases are failing with same reason. Please note this is happening on master (without my changes) as well. Will unblock myself by skipping. I didn't do that thinking something went wrong with my code base after rebase to dotnet/master (including hostpolicy.dll) until I found it happening on master too. I am using Microsoft Window 10 Pro version Version 10.0.17134 Build 17134. I was not sure hence didn't file an issue.

That particular test is trying to make sure the options cache is working as expected so there might be a bug that only reproduces intermittently. If it is consistently failing, can you please share the whole call stack as well, because I wasn't able to repro this issue (and it isn't happening on CI either).

Please find whole output here in a gist file.

I can try checking out your branch to see, is this it: https://github.com/WinCPP/corefx/tree/newtrygets?

Yes above is the branch with latest changes (I just pushed them, please note they are not tested). However the above gist was captured using master branch https://github.com/WinCPP/corefx. I have not rebased it again with dotnet/master after this morning.

WinCPP · 2019-06-24T16:41:35Z

@ahsonkhan please refer to above reply.

After marking the failing tests as ActiveIssue on my local code base, the other tests passed. Are the issues that I see legit? In that case I will open a new issue and check in the failing test cases with active issue number. Appreciate your thoughts.

In the automated build corefx-ci on Linux x64_Debug and Windows x64_Debug are consistently failing in http functional and security related tests. Not sure how to proceed there too.

ahsonkhan · 2019-06-24T17:21:22Z

Are the issues that I see legit?

Given they are not failing in CI, and I can't repro it locally either, it doesn't look like an issue. There may be some issue with the culture setting on your machine (where date times are being formatted differently), but I am not sure.

One thing that might be worth trying is debug/output the DateTime value of the object being serialized from one of the tests. It should be in the correct (ISO) format. For instance, at this point, what does SimpleTestClass.MyDateTime and SimpleTestClass.MyDateTimeOffset look like (are they still 2019-01-30T12:01:02.0000000Z, 2019-01-30T12:01:02.0000000+01:00)?

corefx/src/System.Text.Json/tests/Serialization/CacheTests.cs

Line 63 in 48fde52

testObj.Verify();

In the automated build corefx-ci on Linux x64_Debug and Windows x64_Debug are consistently failing in http functional and security related tests. Not sure how to proceed there too.

There are issues tracking these, so we can ignore them for now.

ahsonkhan · 2019-06-24T19:19:01Z

Failures are unrelated:
https://github.com/dotnet/corefx/issues/37362
https://github.com/dotnet/corefx/issues/38836
https://github.com/dotnet/corefx/issues/38837

…et/corefx#38469) * CoreFx dotnet/corefx#36125 (Try)Get for byte sbyte short ushort * Init out values on failure return paths. * APIs in JsonElement and related tests * More test cases, test bug fixes, code fixes Commit migrated from dotnet/corefx@b2af402

WinCPP force-pushed the newtrygets branch from b40f1d0 to 1b268e7 Compare June 11, 2019 22:28

ahsonkhan added the area-System.Text.Json label Jun 12, 2019

ahsonkhan added this to the 3.0 milestone Jun 12, 2019

watfordgnf reviewed Jun 12, 2019

View reviewed changes

src/System.Text.Json/src/System/Text/Json/Reader/Utf8JsonReader.TryGet.cs Outdated Show resolved Hide resolved

WinCPP force-pushed the newtrygets branch from 651e6c6 to 09d619e Compare June 12, 2019 19:11

stephentoub reviewed Jun 12, 2019

View reviewed changes

src/System.Text.Json/src/System/Text/Json/Reader/Utf8JsonReader.TryGet.cs Show resolved Hide resolved

bartonjs reviewed Jun 17, 2019

View reviewed changes

ahsonkhan reviewed Jun 18, 2019

View reviewed changes

WinCPP force-pushed the newtrygets branch from 55fdc18 to 29a7113 Compare June 23, 2019 19:42

WinCPP added 4 commits June 24, 2019 22:13

CoreFx #36125 (Try)Get for byte sbyte short ushort

2564824

Init out values on failure return paths.

827c036

APIs in JsonElement and related tests

ffb1fbf

More test cases, test bug fixes, code fixes

9eff6fb

WinCPP force-pushed the newtrygets branch from 29a7113 to 9eff6fb Compare June 24, 2019 16:44

ahsonkhan approved these changes Jun 24, 2019

View reviewed changes

ahsonkhan assigned WinCPP Jun 24, 2019

ahsonkhan merged commit b2af402 into dotnet:master Jun 25, 2019

ahsonkhan mentioned this pull request Jun 26, 2019

Add JsonDocumentOptions and use that instead of JsonReaderOptions. #38866

Merged

WinCPP deleted the newtrygets branch September 9, 2019 03:38

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

CoreFx #36125 (Try)Get for byte sbyte short ushort #38469

CoreFx #36125 (Try)Get for byte sbyte short ushort #38469

WinCPP commented Jun 11, 2019 •

edited

Loading

stephentoub Jun 12, 2019

stephentoub Jun 12, 2019

WinCPP Jun 13, 2019

bartonjs Jun 17, 2019

WinCPP Jun 17, 2019

ahsonkhan Jun 18, 2019

danmoseley Jun 19, 2019

ahsonkhan Jun 19, 2019 •

edited

Loading

WinCPP Jun 22, 2019

ahsonkhan Jun 22, 2019

WinCPP commented Jun 15, 2019

bartonjs Jun 17, 2019

WinCPP Jun 17, 2019

ahsonkhan Jun 18, 2019 •

edited

Loading

bartonjs Jun 17, 2019

WinCPP Jun 17, 2019

ahsonkhan Jun 18, 2019

ahsonkhan Jun 18, 2019

ahsonkhan Jun 18, 2019

WinCPP Jun 19, 2019

ahsonkhan Jun 23, 2019

WinCPP Jun 23, 2019

ahsonkhan Jun 18, 2019

danmoseley commented Jun 22, 2019

ahsonkhan commented Jun 22, 2019

WinCPP commented Jun 22, 2019

ahsonkhan commented Jun 22, 2019 •

edited

Loading

bartonjs commented Jun 22, 2019

WinCPP commented Jun 23, 2019

ahsonkhan commented Jun 23, 2019

WinCPP commented Jun 23, 2019

WinCPP commented Jun 24, 2019

ahsonkhan commented Jun 24, 2019

ahsonkhan commented Jun 24, 2019

	public uint GetUInt16() { throw null; }
	public ushort GetUInt16() { throw null; }

CoreFx #36125 (Try)Get for byte sbyte short ushort #38469

CoreFx #36125 (Try)Get for byte sbyte short ushort #38469

Conversation

WinCPP commented Jun 11, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ahsonkhan Jun 19, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

WinCPP commented Jun 15, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ahsonkhan Jun 18, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

danmoseley commented Jun 22, 2019

ahsonkhan commented Jun 22, 2019

WinCPP commented Jun 22, 2019

ahsonkhan commented Jun 22, 2019 • edited Loading

bartonjs commented Jun 22, 2019

WinCPP commented Jun 23, 2019

ahsonkhan commented Jun 23, 2019

WinCPP commented Jun 23, 2019

WinCPP commented Jun 24, 2019

ahsonkhan commented Jun 24, 2019

ahsonkhan commented Jun 24, 2019

WinCPP commented Jun 11, 2019 •

edited

Loading

ahsonkhan Jun 19, 2019 •

edited

Loading

ahsonkhan Jun 18, 2019 •

edited

Loading

ahsonkhan commented Jun 22, 2019 •

edited

Loading