Make `Excluding()` work on nested collections if root is a collection #2135

whymatter · 2023-03-04T15:34:09Z

Fixes #1989 #1919

I removed the !ContainsIndexingQualifiers(selectedPath) check on this piece of code which was introduced here: aab3303#diff-53539b5c2a4cdb7276ff5e63cbe410a164938d3620ebf08f20797810d22b394c. I am 100% unsure what it does and what the implications are of me removing it. However, there are no failing tests with the condition removed.

if (!ContainsIndexingQualifiers(selectedPath))  // <-- removed this
{
    currentPath = RemoveIndexQualifiers(currentPath);
}

If removing this condition is bad for whatever reason, please tell me which test case I can add to cover this.

IMPORTANT

If the PR touches the public API, the changes have been approved in a separate issue with the "api-approved" label.
The code complies with the Coding Guidelines for C#.
The changes are covered by unit tests which follow the Arrange-Act-Assert syntax and the naming conventions such as is used in these tests.
If the PR adds a feature or fixes a bug, please update the release notes with a functional description that explains what the change means to consumers of this library, which are published on the website.
If the PR changes the public API the changes needs to be included by running AcceptApiChanges.ps1 or AcceptApiChanges.sh.
If the PR affects the documentation, please include your changes in this pull request so the documentation will appear on the website.
- Please also run ./build.sh --target spellcheck or .\build.ps1 --target spellcheck before pushing and check the good outcome

… Excluding() work on nested collections if root is a collection

whymatter · 2023-03-04T15:39:24Z

Src/FluentAssertions/Equivalency/Selection/SelectMemberByPathSelectionRule.cs

-
-        // If we're part of a collection comparison, the selected path will not include an index,
-        // so we need to remove it from the current node as well.
-        if (!ContainsIndexingQualifiers(selectedPath))


This is the part where I am not sure about the implications, since all tests are green

In trying to confirm this, I tried checking out 77b5f2c and run the tests.
I got the same results with and without the regex replacements in PropertyPathOrderingRule, ExcludePropertyByPathSelectionRule and IgnorePropertySelectionRule.

In develop branch in PathBasedOrderingRule I can remove the all the IndexQualifier stuff completely without any tests failing.
So something similar might be going on there?

coveralls · 2023-03-04T15:46:03Z

Pull Request Test Coverage Report for Build 4493934804

Warning: This coverage report may be inaccurate.

This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.

For more information on this, see Tracking coverage changes with pull request builds.
To avoid this issue with future PRs, see these Recommended CI Configurations.
For a quick fix, rebase this PR at GitHub. Your next report should be accurate.

Details

1 of 1 (100.0%) changed or added relevant line in 1 file are covered.
3 unchanged lines in 1 file lost coverage.
Overall coverage remained the same at 97.151%

Files with Coverage Reduction	New Missed Lines	%
Src/FluentAssertions/Equivalency/Steps/AutoConversionStep.cs	3	86.36%

Totals
Change from base Build 4391754480:	0.0%
Covered Lines:	12820
Relevant Lines:	13048

💛 - Coveralls

dennisdoomen · 2023-03-05T08:15:38Z

I removed the !ContainsIndexingQualifiers(selectedPath) check on this piece of code which was introduced here: aab3303#diff-53539b5c2a4cdb7276ff5e63cbe410a164938d3620ebf08f20797810d22b394c. I am 100% unsure what it does and what the implications are of me removing it. However, there are no failing tests with the condition removed.

It was necessary to normalize an expression containing an index qualifier like root.Collection[0].Property to []. You can still use a string with a nested path or an expression to select members to include and exclude. I'm embarrassed to not see a test in that commit though.

whymatter · 2023-03-06T15:26:37Z

Hmm, I don´t understand it. RemoveIndexQualifiers does only something if the root is a collection, right? So that the following is true

RemoveIndexQualifiers("root[0].Foo[2].Bar") = "root.Foo[2].Bar" // root is collection
RemoveIndexQualifiers("root.Foo[2].Bar") => "root.Foo[2].Bar".  // root is no collection

The if condition I removed prevents the first case from happening if I add an exclusion like x => x.Foo[2].Bar. Why?
What is a test case for the if condition? I don't get it :(

dennisdoomen · 2023-03-07T15:15:36Z

Hmm, I don´t understand it. RemoveIndexQualifiers does only something if the root is a collection, right?

No, it was called when the path contained any [ and ], regardless of the root. This was used in expressions, not strings.

So opt.Excluding(x => x.Foo[2].Bar) would get normalized to root.Foo[].Bar, which is the old way of doing opt.For(x => x.Foo).Excluding(y => .Bar).

whymatter · 2023-03-08T20:11:08Z

So is my PR fine then?

dennisdoomen · 2023-03-09T07:00:50Z

So is my PR fine then?

I'm not sure anymore if my memory is serving me well. But the issue was that if we were using BeEquivalentTo on a collection, the caller could provide the property path for including or excluding a property without the first index (e.g. root[0]). Instead, it would specify the path that assumes an item of the collection. But the actual path of the node in the graph will include root[0] or something like that.

dennisdoomen · 2023-03-09T12:09:21Z

Also, if I remove that part, I have about 30 tests failing

whymatter · 2023-03-10T15:11:03Z

Also, if I remove that part, I have about 30 tests failing

I assume you removed the line currentPath = RemoveIndexQualifiers(currentPath); as well? I just removed the if condition so that currentPath = RemoveIndexQualifiers(currentPath); executes all the time

dennisdoomen · 2023-03-10T15:37:43Z

I assume you removed the line currentPath = RemoveIndexQualifiers(currentPath); as well? I just removed the if condition so that currentPath = RemoveIndexQualifiers(currentPath); executes all the time

Yeah, I removed the entire if statement.

whymatter · 2023-03-10T16:05:09Z

I assume you removed the line currentPath = RemoveIndexQualifiers(currentPath); as well? I just removed the if condition so that currentPath = RemoveIndexQualifiers(currentPath); executes all the time

Yeah, I removed the entire if statement.

Okay, so because currentPath = RemoveIndexQualifiers(currentPath); is definitely required.
However, I do not understand if and why the if surrounding it is required? (Especially because removing the if resolves the bug I am working on)

dennisdoomen · 2023-03-11T13:26:08Z

However, I do not understand if and why the if surrounding it is required? (Especially because removing the if resolves the bug I am working on)

Me neither. So if all the tests succeed, I guess this should be good enough. I can't think of a scenario where we would not want to remove the starting [0]

Src/FluentAssertions/Equivalency/Selection/SelectMemberByPathSelectionRule.cs

# Conflicts: # Src/FluentAssertions/Equivalency/Selection/SelectMemberByPathSelectionRule.cs

Src/FluentAssertions/Equivalency/Selection/SelectMemberByPathSelectionRule.cs

docs/_pages/releases.md

Co-authored-by: Jonas Nyrup <jnyrup@users.noreply.github.com>

jnyrup

I find it really weird that we have code that apparently never had any purpose according to our tests.
I'm fine with doing this and then we must release a another patch version if we ended up breaking a case.

whymatter added 3 commits March 4, 2023 16:18

fluentassertions#1989 Remove ContainsIndexingQualifiers check to make…

8e86c09

… Excluding() work on nested collections if root is a collection

fluentassertions#1989 Added test case for For().Exclude()

ab61c36

fluentassertions#1989 Adapted releases.md

f46ff14

whymatter commented Mar 4, 2023

View reviewed changes

dennisdoomen approved these changes Mar 11, 2023

View reviewed changes

Src/FluentAssertions/Equivalency/Selection/SelectMemberByPathSelectionRule.cs Outdated Show resolved Hide resolved

whymatter added 2 commits March 12, 2023 18:02

fluentassertions#1989 Refactor rename RemoveRootIndexQualifier

d20143f

Merge branch 'develop' into issue-1989

dd157ad

# Conflicts: # Src/FluentAssertions/Equivalency/Selection/SelectMemberByPathSelectionRule.cs

whymatter marked this pull request as ready for review March 12, 2023 17:11

jnyrup reviewed Mar 22, 2023

View reviewed changes

Src/FluentAssertions/Equivalency/Selection/SelectMemberByPathSelectionRule.cs Show resolved Hide resolved

docs/_pages/releases.md Outdated Show resolved Hide resolved

Update docs/_pages/releases.md

083ee9d

Co-authored-by: Jonas Nyrup <jnyrup@users.noreply.github.com>

whymatter requested a review from jnyrup March 22, 2023 22:30

jnyrup linked an issue Mar 26, 2023 that may be closed by this pull request

Excluding properties behind an array doesn't work in BeEquivalentTo on collections #1919

Closed

jnyrup approved these changes Mar 30, 2023

View reviewed changes

dennisdoomen merged commit 1829408 into fluentassertions:develop Apr 1, 2023

jnyrup added the bug label Apr 10, 2023

jnyrup mentioned this pull request Apr 10, 2023

.For(x => x.[...]).Exclude(x => x.[...]) doesn't exclude properties for nested collections #2177

Closed

jnyrup changed the title ~~Make Excluding() work on nested collections if root is a collection~~ Make Excluding() work on nested collections if root is a collection Apr 15, 2023

This was referenced Sep 10, 2025

Bump FluentAssertions from 6.4.0 to 7.2.0 dotnet/reactive#2233

Open

Bump FluentAssertions from 5.10.3 to 8.6.0 openwallet-foundation-labs/wallet-framework-dotnet#386

Closed

Bump FluentAssertions from 5.10.3 to 8.6.0 deepak01011/WorkSyncBackend#10

Closed

dependabot bot mentioned this pull request Oct 13, 2025

Bump FluentAssertions from 5.10.3 to 8.7.1 openwallet-foundation-labs/wallet-framework-dotnet#421

Closed

This was referenced Oct 27, 2025

Bump FluentAssertions from 5.10.3 to 8.8.0 openwallet-foundation-labs/wallet-framework-dotnet#441

Closed

Bump FluentAssertions from 5.10.3 to 8.8.0 openwallet-foundation-labs/wallet-framework-dotnet#445

Closed

This was referenced Nov 10, 2025

Bump FluentAssertions from 5.10.3 to 8.8.0 openwallet-foundation-labs/wallet-framework-dotnet#452

Closed

Bump FluentAssertions from 5.10.3 to 8.8.0 openwallet-foundation-labs/wallet-framework-dotnet#469

Closed

This was referenced Nov 24, 2025

Bump FluentAssertions from 5.10.3 to 8.8.0 openwallet-foundation-labs/wallet-framework-dotnet#476

Closed

Bump FluentAssertions from 5.10.3 to 8.8.0 jleachenski/AspNetCore-DDD#12

Open

This was referenced Dec 22, 2025

Bump FluentAssertions from 5.10.3 to 8.8.0 petrikero/akka.net#34

Open

Bump FluentAssertions from 5.10.3 to 8.8.0 ChrisKXu/todl#122

Closed

dependabot bot mentioned this pull request Jan 1, 2026

Bump FluentAssertions from 6.10.0 to 8.8.0 JensKrumsieck/PorphyStruct#126

Open

This was referenced Jan 11, 2026

Bump FluentAssertions from 5.10.3 to 8.8.0 thashiznit2003/Bookdarr#9

Merged

Bump FluentAssertions from 6.4.0 to 8.8.0 angeldev99/dotnet-reactive#2

Open

Make Excluding() work on nested collections if root is a collection #2135

Make Excluding() work on nested collections if root is a collection #2135

Uh oh!

Conversation

whymatter commented Mar 4, 2023

IMPORTANT

Uh oh!

whymatter Mar 4, 2023

Choose a reason for hiding this comment

Uh oh!

jnyrup Mar 25, 2023

Choose a reason for hiding this comment

Uh oh!

coveralls commented Mar 4, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pull Request Test Coverage Report for Build 4493934804

Warning: This coverage report may be inaccurate.

Details

💛 - Coveralls

Uh oh!

dennisdoomen commented Mar 5, 2023

Uh oh!

whymatter commented Mar 6, 2023

Uh oh!

dennisdoomen commented Mar 7, 2023

Uh oh!

whymatter commented Mar 8, 2023

Uh oh!

dennisdoomen commented Mar 9, 2023

Uh oh!

dennisdoomen commented Mar 9, 2023

Uh oh!

whymatter commented Mar 10, 2023

Uh oh!

dennisdoomen commented Mar 10, 2023

Uh oh!

whymatter commented Mar 10, 2023

Uh oh!

dennisdoomen commented Mar 11, 2023

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jnyrup left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Make `Excluding()` work on nested collections if root is a collection #2135

Make `Excluding()` work on nested collections if root is a collection #2135

coveralls commented Mar 4, 2023 •

edited

Loading