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

Fix false-positive duplicate detection in mapping deep member paths to TVP columns #488

Merged
merged 2 commits into from
Dec 28, 2022

Conversation

sharpjs
Copy link
Contributor

@sharpjs sharpjs commented Dec 9, 2022

Description

This PR deals with mapping deep member paths to table-valued parameter columns. Suppose a custom column mapper creates the following mappings:

TVP Column Member Path
A_X A.X
B_X B.X

The column-mapping code attempts to deduplicate mappings by keeping a HashSet of used members. When the code finds a member in the HashSet, the code knows that a mapping used the member already. The code then avoids another mapping to the member.

The key for duplicate detection is the ClassPropInfo of the final member in the member path. This causes false-positive duplicate detection in several scenarios, including:

  • TVP with multiple children of the same type

    class Parent
    {
        public Child A;
        public Child B;
    }
    
    class Child
    {
        public int X;
    }
  • TVP with multiple children of different types that each inherit a property from the same base class

    class Parent
    {
        public ChildA A;
        public ChildB B;
    }
    
    class ChildA : Child { /*...*/ }
    class ChildB : Child { /*...*/ }
    
    abstract class Child
    {
        public int X;
    }

This PR supports such scenarios by changing the duplicate detection key to the member path itself.

Checklist

Please make sure your pull request fulfills the following requirements:

  • Tests for any changes have been added (for bug fixes / features).
  • Docs have been added / updated (for bug fixes / features).
    - As far as I know, no documentation needs to be changed.

Type

This pull request includes what type of changes?

  • Bug.
  • Feature.
  • Code style update (formatting, local variables).
  • Refactoring (no functional changes, no api changes).
  • Documentation content changes.
  • Other (please describe below).

Breaking Changes

Does this pull request introduce any breaking changes?

  • Yes
  • No ... unless you depend on arguably buggy behaviour

This change causes parameter data to flow to TVP columns where it previously has not.

Any other comment

(n/a)

This resolves a false duplicate detection when different deep member
paths resolve to the same final member but in separate instances.
@sharpjs sharpjs changed the title Deep path dedupe Fix false-positive duplicate detection in mapping deep member paths to TVP columns Dec 9, 2022
@jonwagner
Copy link
Owner

awesome! I have a holiday break coming up so it's about time for a new build

Copy link
Owner

@jonwagner jonwagner left a comment

Choose a reason for hiding this comment

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

+1

@jonwagner jonwagner merged commit 24daa65 into jonwagner:main Dec 28, 2022
@jonwagner
Copy link
Owner

alrighty 6.3.11 is out

@sharpjs sharpjs deleted the deep-path-dedupe branch January 30, 2023 22:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants