bugfix: improve fallback when StringEnum encounters null value

[shiftkey]

Resolves #2052

Testing

To aid with testing this PR, you can add a NuGet.config file to the root of your project that adds our beta packages feed:

<configuration>
    <packageSources>
        <add key="nuget.org" value="https://api.nuget.org/v3/index.json" protocolVersion="3" />
        <add key="Octokit beta" value=" https://ci.appveyor.com/nuget/octokit-net" />
    </packageSources>
</configuration>

Then, switch your version to the beta release just published, currently 0.45.0-fix-teams-deseri0005, and try to verify the fix. I'm still working to try and reproduce this on my side, but I have an idea of where it's lurking...

[codecov]

Codecov Report

Merging #2156 into master will increase coverage by 0.03%.
The diff coverage is 75%.

@@            Coverage Diff             @@
##           master    #2156      +/-   ##
==========================================
+ Coverage   66.73%   66.77%   +0.03%     
==========================================
  Files         545      545              
  Lines       14233    14235       +2     
==========================================
+ Hits         9498     9505       +7     
+ Misses       4735     4730       -5

Impacted Files	Coverage Δ
Octokit/Http/SimpleJsonSerializer.cs	95.95% <100%> (+0.08%)	⬆️
Octokit/Models/Response/Team.cs	10% <50%> (+10%)	⬆️
Octokit/SimpleJson.cs	70.41% <0%> (+0.26%)	⬆️

[shiftkey]

@cliffchapmanrbx @MikhailTymchukDX are either of you able to verify this PR fixes the problem for you? I'd love to be able to confirm this is good before publishing an update to everyone...

[cliffchapmanrbx]

I used null in my code s well as for the ParameterAttribute for the enum. PermissionLevel appears to use the word none as its value though. Do you need to change this to be none instead of null?

[shiftkey]

I prefer null here to explicitly indicate no value was found, and I want this to be different to the none scenario that might be valid here.

In the test itself you'll see that I can't parse out a valid value from this enum.

Assert.Equal("null", result.Parent.Permission.StringValue);

PermissionLevel value;
Assert.False(result.Parent.Permission.TryParse(out value));

This fixes the deserialization issue, and leverages our existing string-like enums to handle unknown values rather than mapping this null to none.

[cliffchapmanrbx]

Ahha, that makes more sense, thanks!

[cliffchapmanrbx]

@shiftkey You'd also need access to our GitHub Enterprise server which might be difficult :)

I added a comment about none vs null, I'm a bit slammed with meetings today (under Bay Area shelter in place order) so I'll try and check this as soon as I can.

[shiftkey]

@cliffchapmanrbx aha yes, that's also helpful context because you might be working against an older version of the GitHub API depending on your version of GHE

[cliffchapmanrbx]

We keep it up to date specifically to avoid those kinds of problems.

Fortunately (?) my next meeting just got cancelled so I've got an hour to play around with this.

[shiftkey]

@cliffchapmanrbx do you know which version of GHE you are working against? This will help me to confirm where it was introduced and whether it's been fixed since.

[cliffchapmanrbx]

2.20.2 as of the middle of last week, previously we were on 2.19.5. I don't know if the upgrade caused this issue, or someone shuffling some teams around and suddenly making them nested was what surfaced it. The original bug report is from December.

[cliffchapmanrbx]

I can confirm that we no longer see the error from repos with nested teams. I appreciate the hustle!

Do you have an ETA on when this might be available as a new version? I can just re-publish the beta package to our internal feed as a temporary workaround if it's not today or tomorrow.

[shiftkey]

Thanks for confirming @cliffchapmanrbx.

The original bug report is from December.

Yeah, I suspect that one was also from GitHub Enterprise but I forgot to ask at the time.

Do you have an ETA on when this might be available as a new version?

Aiming for tonight once I've cleared out some other work.

[shiftkey]

release_notes: Fix for deserializing issue when parent team has null set for Permission in some GitHub Enterprise environments.

[shiftkey]

@cliffchapmanrbx the fix has been released https://github.com/octokit/octokit.net/releases/tag/v0.45.0 and is being indexed on NuGet as we speak...

[cliffchapmanrbx]

I just updated my local copy to 0.45.0 and can confirm it is still good. Rolling it out to our internal services now. I really appreciate the quick turnaround on this once we had it isolated 👍

[StingyJack]

this makes Value 'pull' is not a valid 'PermissionLevel' enum value get thrown when trying to serialize a repo object with codeowners that includes a GH team