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.

Sign up for GitHub

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

Jump to bottom

Properly reject NuGet newline-only changes. #10332

Merged
merged 7 commits into from
Aug 1, 2024
Merged

Properly reject NuGet newline-only changes. #10332

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
ec6bac4
`only_deleted_lines?` should ignore white-space lines
rhyskoedijk Jul 31, 2024
cf59003
Fix linting
rhyskoedijk Jul 31, 2024
f4ca9a3
Add unit tests
rhyskoedijk Jul 31, 2024
6743e98
Restore original line spacing
rhyskoedijk Jul 31, 2024
f834d5c
test line content comparison directly
brettfo Jul 31, 2024
aa6369d
add test case; deleting a line => changes detected
brettfo Jul 31, 2024
31e7974
Merge branch 'main' into dev/brettfo/nuget-reject-whitespace-changes
abdulapopoola Aug 1, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
24 changes: 15 additions & 9 deletions nuget/lib/dependabot/nuget/file_updater.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,20 @@ def self.updated_files_regex
]
end

sig { params(original_content: T.nilable(String), updated_content: String).returns(T::Boolean) }
def self.differs_in_more_than_blank_lines?(original_content, updated_content)
# Compare the line counts of the original and updated content, but ignore lines only containing white-space.
# This prevents false positives when there are trailing empty lines in the original content, for example.
original_lines = (original_content&.lines || []).map(&:strip).reject(&:empty?)
updated_lines = updated_content.lines.map(&:strip).reject(&:empty?)

# if the line count differs, then something changed
return true unless original_lines.count == updated_lines.count

# check each line pair, ignoring blanks (filtered above)
original_lines.zip(updated_lines).any? { |pair| pair[0] != pair[1] }
end

sig { override.returns(T::Array[Dependabot::DependencyFile]) }
def updated_dependency_files
base_dir = "/"
Expand All @@ -45,7 +59,7 @@ def updated_dependency_files
normalized_content = normalize_content(f, updated_content)
next if normalized_content == f.content

next if only_deleted_lines?(f.content, normalized_content)
next unless FileUpdater.differs_in_more_than_blank_lines?(f.content, normalized_content)
Copy link
Contributor

Choose a reason for hiding this comment

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

Does the original scenario (#9162) still pass here?

If the updated content has less lines than the original content, then a package has been removed somewhere and we likely don't want to create a pull request.

Maybe there needs to be a test for this as well, unless there are other checks elsewhere to deal with this problem now.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good call, that was a test case I missed. I just pushed a commit that adds this test case if you want to double check I understood the issue.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Re-reading your comment I think there is a separate issue to address; what if a package is removed? I think that should be handled in the C# code that actually checks the file content. This part of the code is to handle silliness around empty lines. There's likely a bigger work item to ensure that a dependency isn't removed, but we have better unit tests in the C# code for that. Do you have a scenario where a package was removed?

Copy link
Contributor

@rhyskoedijk rhyskoedijk Jul 31, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

My understanding of the description in #9162 is that if you had a project like this:

<Project Sdk="Microsoft.NET.Sdk">
  <PropertyGroup>
    <TargetFramework>net461</TargetFramework>
  </PropertyGroup>
  <ItemGroup>
    <PackageReference Include="DependencyA" Version="1.0.0" />
  </ItemGroup>
</Project>

And NuGetUpdater attempted to update "DependencyA" to "2.0.0" but failed and rolled back e.g.

WARNING: Install failed. Rolling back...

It would remove the dependency entirely and result in a project file like:

~~~
@@ -2,7 +2,4 @@
   <PropertyGroup>
     <TargetFramework>net461</TargetFramework>
   </PropertyGroup>
-  <ItemGroup>
-    <PackageReference Include="DependencyA" Version="1.0.0" />
-  </ItemGroup>
 </Project>
\ No newline at end of file

In this scenario, the file is different, but it is different in a way that the PR should not be created because the change isn't desired. I could be misunderstanding this problem though.

Maybe the original issue shouldn't have been solved in file_updater and instead the "rollback" warning should be escalated as an error by the native NuGet updater instead? Maybe this scenario isn't even valid anymore, in which case, ignore me.

I'll let you decide what is correct here as you understand this all better than me.
Ultimately this change solves my specific problem, so I'm happy with it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'd say the scenario you presented should be handled purely in C#. Let's take this PR as is, because it does fix a class of errors that we're currently seeing and I'll look at the rollback scenario tomorrow and see if I can nail down what might cause that. There appears to be enough information in the linked issue to sort it out.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@rhyskoedijk I filed issue #10342 to track this other scenario and I'm working on it now. Rough ETA, sometime next week. I think the best mechanism to prevent this kind of error is to only report available package upgrades if all transitive dependencies can be resolved so that we never try to update when it's not fully resolvable (the base scenario that #9162 was trying to address).


puts "The contents of file [#{f.name}] were updated."

Expand Down Expand Up @@ -217,14 +231,6 @@ def check_required_files

raise "No project file or packages.config!"
end

sig { params(original_content: T.nilable(String), updated_content: String).returns(T::Boolean) }
def only_deleted_lines?(original_content, updated_content)
original_lines = original_content&.lines || []
updated_lines = updated_content.lines

original_lines.count > updated_lines.count
end
end
end
end
Expand Down
163 changes: 147 additions & 16 deletions nuget/spec/dependabot/nuget/file_updater_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -161,22 +161,6 @@ def intercept_native_tools(discovery_content_hash:)
)
end
end

context "when the file has only deleted lines" do
before do
allow(File).to receive(:read)
.and_call_original
allow(File).to receive(:read)
.with("#{repo_contents_path}/Proj1/Proj1/Proj1.csproj")
.and_return("")
end

it "does not update the project" do
run_update_test do |updater|
expect(updater.updated_dependency_files.map(&:name)).to be_empty
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This test case was wrong; it was the case where a file was updated and a trailing newline was removed and it was erroneously detected as just deleting the trailing newline, which wasn't correct because the contents actually changed. This scenario is covered in the new tests below.

end
end
end
end
end

Expand Down Expand Up @@ -262,4 +246,151 @@ def intercept_native_tools(discovery_content_hash:)
end
end
end

describe "#differs_in_more_than_blank_lines?" do
subject(:result) { described_class.differs_in_more_than_blank_lines?(original_content, updated_content) }

context "when the original content is `nil` and updated is empty" do
let(:original_content) { nil }
let(:updated_content) { "" }

it { is_expected.to be(false) }
end

context "when the original content is `nil` and updated is non-empty" do
let(:original_content) { nil }
let(:updated_content) { "line1\nline2" }

it { is_expected.to be(true) }
end

context "when there is a difference with no blank lines" do
let(:original_content) do
<<~TEXT
original-line-1
original-line-2
original-line-3
TEXT
end
let(:updated_content) do
<<~TEXT
original-line-1
UPDATED-LINE-2
original-line-3
TEXT
end

it { is_expected.to be(true) }
end

context "when there is a difference with blank lines" do
let(:original_content) do
<<~TEXT
original-line-1

original-line-2
original-line-3
TEXT
end
let(:updated_content) do
<<~TEXT
original-line-1

UPDATED-LINE-2
original-line-3
TEXT
end

it { is_expected.to be(true) }
end

context "when a blank line was added" do
let(:original_content) do
<<~TEXT
original-line-1
original-line-2
original-line-3
TEXT
end
let(:updated_content) do
<<~TEXT
original-line-1

original-line-2
original-line-3
TEXT
end

it { is_expected.to be(false) }
end

context "when a blank line was removed, but no other changes" do
let(:original_content) do
<<~TEXT
original-line-1

original-line-2
original-line-3
TEXT
end
let(:updated_content) do
<<~TEXT
original-line-1
original-line-2
original-line-3
TEXT
end

it { is_expected.to be(false) }
end

context "when a blank line was removed and another was changed" do
let(:original_content) do
<<~TEXT
original-line-1

original-line-2
original-line-3
TEXT
end
let(:updated_content) do
<<~TEXT
original-line-1
UPDATED-LINE-2
original-line-3
TEXT
end

it { is_expected.to be(true) }
end

context "when a line was added and blank lines are present" do
let(:original_content) do
<<~TEXT
original-line-1

original-line-2
original-line-3
TEXT
end
let(:updated_content) do
<<~TEXT
original-line-1

original-line-2
SOME-NEW-LINE
original-line-3
TEXT
end

it { is_expected.to be(true) }
end

context "when the only difference is a trailing newline" do
let(:original_content) { "line-1\nline-2\n" }
let(:updated_content) { "line-1\nline-2" }

it { is_expected.to be(false) }
end
end
end
Loading