NuGet.Protocol nullability - Phase 1

[nkolev92]

Progress on: NuGet/Home#14851

Description

Remove #nullable disable from 24 leaf-type files in NuGet.Protocol. Most changes are mechanical — the interesting decisions are below.

Significant decisions

JSON model nullability driven by NuGet V3 protocol spec:

Type	Property	Spec	Annotation	Rationale
AlternatePackageMetadata	PackageId	required	string?	3rd party servers may omit it
AlternatePackageMetadata	Range	optional	VersionRange?
PackageDeprecationMetadata	Reasons	required	Array.Empty<string>() default	Safer for 3rd party servers that may omit it
PackageDeprecationMetadata	Message	optional	string?
PackageDeprecationMetadata	AlternatePackage	optional	AlternatePackageMetadata?

RepositoryCertificateInfo — all fields Required.Always per repo signatures spec. Risk is minimal since this is package signing infrastructure declared in one place on nuget.org, and IRepositoryCertificateInfo is already declared in NuGet.Packaging.

PackageProgressEventArgs.PackageSource → PackageSource? — constructor accepts null, HasPackageSource checks for it. Also marked [Obsolete] since it has zero usages in the codebase.

VersionInfo.PackageSearchMetadata → IPackageSearchMetadata? — XML doc explicitly states it's null for V3 sources.

VersionRangeConverter.WriteJson — explicit ArgumentNullException guard instead of !-suppression on the nullable value parameter.

Downstream fix

PackageSearchResultTableRenderer.cs — PackageDeprecationMetadata.Message is now string?, added ?? "N/A" fallback.

Mechanical changes

Everything else is removal of #nullable disable from types where all members are provably non-null: constants, exception hierarchies, event types with constructor-set properties, and models with constructor validation. 65 oblivious (~) entries updated in PublicAPI.Shipped.txt for both TFMs.

PR Checklist

• Meaningful title, helpful description and a linked NuGet/Home issue
• Added tests
• Link to an issue or pull request to update docs if this PR changes settings, environment variables, new feature, etc.

[nkolev92]

@joelverhagen

I am making serialization stricter here.
I am largely relying on the protocol documentation.
Are there any chances that NuGet.org doesn't implement all these properties correctly?

@NuGet/nuget-client Similar question to all of you. Any concerns with the "risky decisions" here?

[joelverhagen]

@joelverhagen

I am making serialization stricter here. I am largely relying on the protocol documentation. Are there any chances that NuGet.org doesn't implement all these properties correctly?

@NuGet/nuget-client Similar question to all of you. Any concerns with the "risky decisions" here?

It's definitely possible. I recommend asking your agent to compare these client-side model changes with the associated server side code (not models, these are not null annotated.

[zivkan]

Similar question to all of you. Any concerns with the "risky decisions" here?

I believe it's possible at least one 3rd party server implementation will not have read the docs, but would have just tried to run a restore, see where NuGet failed, update their implementation, and repeat until restore passed. So, anything the docs say must be non null, but isn't enforced by client, I think is likely to be null in at least one server implementation.

I haven't yet looked at this PR, but my question is how it's handled. If we're annotating it as not null, but not changing the deserializer to enforce it, then we'll end up with NRE crashes on types we've marked as not null, which might make it more difficult to investigate. If we change the deserializer to start enforcing not null on something that wasn't previously enforced, we'll break customers.

My gut feeling is that we should probably mark all our deserialization class properties as nullable, and we'll just have to deal with it in all our code.

Maybe another approach is start enforcing not null in deserialization, but add a NUGET_UNSUPPORTED_ALLOW_PROTOCOL_ERRORS environment variable for customers to opt out, until they can migrate to a different server implementation that isn't broken. If a future version of NuGet starts throwing NRE and customers complain, we can say that the environment variable calls out that it's unsupported. This idea would need to be validated with the breaking change process.

[nkolev92]

It's definitely possible. I recommend asking your agent to compare these client-side model changes with the associated server side code (not models, these are not null annotated.

@joelverhagen I'll try that. I had it rely on the documentation. I don't know how succesfull it'll be at looking at the server impl.

I believe it's possible at least one 3rd party server implementation will not have read the docs, but would have just tried to run a restore, see where NuGet failed, update their implementation, and repeat until restore passed. So, anything the docs say must be non null, but isn't enforced by client, I think is likely to be null in at least one server implementation.

I guess I'd need to dig deeper there to see whether these types having null would break client. That changes the bar.
fwiw, the types in here are very likely not used by 3rd party, since they are repo signatures etc, but your point stands.

I haven't yet looked at this PR, but my question is how it's handled. If we're annotating it as not null, but not changing the deserializer to enforce it, then we'll end up with NRE crashes on types we've marked as not null, which might make it more difficult to investigate. If we change the deserializer to start enforcing not null on something that wasn't previously enforced, we'll break customers.

The changes here change the deserializer.

My gut feeling is that we should probably mark all our deserialization class properties as nullable, and we'll just have to deal with it in all our code.

I genuinely think this will make the nullability gains disappear, because we're gonna be suppressing nullability warnings in so many places.

We can chat offline. I think there's something to gating changes behind env var. I'll think about it.

[nkolev92]

Looking at the 3 types:

• AlternatePackageMetadata: I think the benefit of the required always here are super small. It's only one type that's being treated as nullable. Honestly this is probably fine as is, let's no go and do super risky things.
• PackageDeprecationMetadata - I could just make reasons default to an empty array. I prefer that honestly.
• RepositoryCertificateInfo - This one is actually declared in NuGet.Packaging as well, and it's package signing related, so I actually think the risky is minimal since this is only declared in one place on nuget.org.

Thoughts?

[nkolev92]

OK, implemented all those changes + updated description.

PTAL.