Fix NRE and Disallow Missing Registration Page Leaves #5741
Conversation
dotnet-policy-service
bot
added
the
Community
|
@dotnet-policy-service agree company="StackOverflow" |
| { | ||
| throw new InvalidDataException(registrationUri.AbsoluteUri); | ||
| log.LogDebug($"{rangeUri} returned 404 NotFound. No versions in this range are available."); |
There was a problem hiding this comment.
Why does the server return a leaf page that 404s?
I think that'd be bad data, so I'm not sure we should continue quietly.
You mentioned 7532d57 had the first bug.
Do you know what was the behavior in 5.6 before this change?
There was a problem hiding this comment.
No not sure how it behaved before that. It 404's because all the versions within the leaf have been unlisted at some point. I can't comment on why cloudsmith opts to 404 when there are no active versions in a range but just based on the IgnoreNotFound flag that's being set here, I feel like this is a valid way of handling this maybe?
There was a problem hiding this comment.
I can force cloudsmith to synchronize at least one package from the range that 404's by explicitly installing an older version. That fixes those urls so they don't 404 for me but I feel like this behavior change between .net sdk 6 and .net sdk 8 is not desirable. Whether that's NuGet's responsibility I'm not sure but this seemed like a reasonable implementation to me.
There was a problem hiding this comment.
It 404's because all the versions within the leaf have been unlisted
I think there's a couple of things that kind of seem incorrect.
- Registration should not be filtering unlisted packages.
- If all packages from a package don't "exist", then the page should not exist either.
In general, I think this change won't really lead to any issues with current servers, since most servers won't have empty pages, but it still feels like this should be a server side fix.
There was a problem hiding this comment.
We tried but couldn't find any way to disable this behavior on the Cloudsmith side. Since we have a workaround in place and this isn't a blocker for us I'm happy to change this PR to just check the correct variable if that's what we want. But I still think this change makes sense in terms of defensive programming.
There was a problem hiding this comment.
I think the check on line 124 makes sense. Currently it's checking the wrong value.
I'm not sure I'd call continue instead of throwing defensive programming. There's a client/server contract and the server is not honoring it. Throwing is a controlled failure.
There was a problem hiding this comment.
If we're going to consider this a data contract error would it make sense to set IgnoreNotFounds to false ?
Not sure why we're handling 404 differently in this case. We can treat it like any other http error code.
There was a problem hiding this comment.
@nkolev92 Let me know what you think of my latest changes.
I set IgnoreNotFounds to false and adjusted my unit test accordingly. I also removed the null check since we ensure a successful http status code in the http handler.
There was a problem hiding this comment.
@nkolev92 Any thoughts on the latest version of the code?
dotnet-policy-service
bot
added
Status:No recent activity
mukunku
changed the title
dotnet-policy-service
bot
added
the
Status:No recent activity
jeffkl
removed
the
Status:No recent activity
dotnet-policy-service
bot
added
the
Status:No recent activity
|
Seems like folks don't like my fix so going to close this out. |
dotnet-policy-service
bot
removed
the
Status:No recent activity
Bug
Fixes: NuGet/Home#13377
Regression? Last working version: The NRE has been there since the code was written in #3462
Description
The original issue is just the wrong variable being used in the
ifcheck. However for some reason when doingdotnet tool restorein the dotnet 6 sdk the issue doesn't happen for us. But once we upgraded our project to use dotnet 8 the NRE started happening every time we randotnet tool restore. So this is sort of a "regression" for us because our in-house NuGet repo (cloudsmith) returns 404 if there are no active packages in a version range.I think it makes sense to gracefully skip any missing leaf pages. We are already allowing 404's with the
IgnoreNotFounds = true,flag. So I think not throwing an exception in a recoverable situation makes more sense here and it would help resolve our issue as well.PR Checklist
PR has a meaningful title
PR has a linked issue.
Described changes
Tests
Documentation