Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Dependencies]: Removes direct reference for Newtonfoft package and add a target build. #4839

Merged

Conversation

aavasthy
Copy link
Contributor

@aavasthy aavasthy commented Oct 22, 2024

Pull Request Template

Description

The intent of this change is to ensure that the Newtonsoft.Json dependency, which is currently used within the SDK, does not propagate to downstream consumers as Package dependency. The SDK references Newtonsoft.Json for its internal functionality, but this package should not be exposed to users of the SDK. This measure also avoids the risk of consumers relying on a specific version of Newtonsoft.Json that might have vulnerabilities or other limitations.

Type of change

  • Dependency Management Update:
    • Make Newtonsoft.Json package reference as private asset.
    • Updated Microsoft.Azure.Cosmos.targets to ensure the Newtonsoft.Json dependency is not passed to consumer apps and there is a build failure when the consumer app do not have Newtonsoft package installed.
    • Provide an option to consumer app to disable this newtonsoft package build failure by setting DisableNewtonsoftJsonCheck to true.
  	<PropertyGroup>
		<DisableNewtonsoftJsonCheck>true</DisableNewtonsoftJsonCheck>
	</PropertyGroup>

Testing the change on a consumer console app with updated dotnet SDK package without installing newtonsoft package in consumer app:

  • Installing the Microsoft.Azure.Cosmos package in the console app:
    • No Newtonsoft package dependency getting added in consumer app.
    • Build failure when no newtonsoft package is available. Failure message "The Newtonsoft.Json package must be explicitly referenced with version >= 10.0.2. Please add a reference to Newtonsoft.Json, or set the 'AzureCosmosDisableNewtonsoftJsonCheck' property to 'true' to bypass this check."
image image image

Once the Newtonsoft nuget package is included the build succeeds:
image

image image

Testing the change on a consumer console app with updated dotnet SDK package when consumer app installs any other version of newtonsoft package on their end:
image

Testing the change on a consumer console app with updated dotnet SDK package when consumer app uses packages.config:
image
image
image
image

Testing the change on a consumer console app with updated dotnet SDK package and Newtonsoft available as transitive dependency:
image
image

Testing the change on a consumer console app with updated dotnet SDK package and build failure and allowing consumer app the option to opt out this build failure due to missing newtonsoft:
image
image

Testing the change on a consumer console app by installing updated Microsoft.Azure.Cosmos.Encryption which has dependency on updated Microsoft.Azure.Cosmos package. The build failure happens if Newtonsoft is missing:
image
image

Please delete options that are not relevant.

  • [] Bug fix (non-breaking change which fixes an issue)
  • [] New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • [] This change requires a documentation update

Closing issues

To automatically close an issue: closes #4674

@bartelink
Copy link
Contributor

Really appreciate the time you took to write the overview (and the fact that it's also very well written!)

@aavasthy aavasthy self-assigned this Oct 24, 2024
@aavasthy aavasthy marked this pull request as ready for review October 24, 2024 21:02
Copy link
Member

@Pilchie Pilchie left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Slight nit in the overview - you should probably check "Breaking Change" instead of non-breaking bug fix.

Microsoft.Azure.Cosmos/src/Microsoft.Azure.Cosmos.targets Outdated Show resolved Hide resolved
Copy link
Member

@Pilchie Pilchie left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agree about putting the condition directly on the whole .

Also, have we verified that this works properly transitively? Specifically, if a library like the Aspire integration package references the Cosmos package, but disables the check to pass the buck on to the consumer, does the consumer correctly get the error?

Microsoft.Azure.Cosmos/src/Microsoft.Azure.Cosmos.targets Outdated Show resolved Hide resolved
@eerhardt
Copy link
Member

Also, have we verified that this works properly transitively?

That's a great point. The .targets file needs to be included/imported from the buildTransitive folder in the nuget package. See https://github.com/NuGet/Home/wiki/Allow-package--authors-to-define-build-assets-transitive-behavior

kirankumarkolli
kirankumarkolli previously approved these changes Nov 5, 2024
Copy link
Member

@eerhardt eerhardt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Thanks!

@kundadebdatta kundadebdatta added the auto-merge Enables automation to merge PRs label Nov 13, 2024
@microsoft-github-policy-service microsoft-github-policy-service bot merged commit aff05fb into master Nov 13, 2024
26 checks passed
@microsoft-github-policy-service microsoft-github-policy-service bot deleted the users/aavasthy/AddBuildTarget_NewtonSoft2 branch November 13, 2024 01:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
auto-merge Enables automation to merge PRs
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Microsoft.Azure.Cosmos references many out of support and vulnerable package versions.
8 participants