efcore icon indicating copy to clipboard operation
efcore copied to clipboard

When creating the "missing column" error message, use the same (case insensitive) column name comparer used in field value retrieval

Open simonmckenzie opened this issue 1 year ago • 2 comments

Issue #33748

This addresses an issue where a column with different casing to the entity's field would be selected as the "first missing column" when assembling the error message. The field name lookup's key comparer is case insensitive, and using it here ensures that all column name comparisons will use the same comparer.

  • [x] I've read the guidelines for contributing and seen the walkthrough
  • [ ] I've posted a comment on an issue with a detailed description of how I am planning to contribute and got approval from a member of the team
  • [x] The code builds and tests pass locally (also verified by our automated build checks)
  • [x] Commit messages follow this format:
        Summary of the changes
        - Detail 1
        - Detail 2

        Fixes #bugnumber
  • [x] Tests for the changes have been added (for bug fixes / features)
  • [x] Code follows the same patterns and style as existing code in this repo

simonmckenzie avatar May 18 '24 01:05 simonmckenzie

@dotnet-policy-service agree

On Sat, 18 May 2024, 12:46 pm dotnet-policy-service[bot], < @.***> wrote:

@simonmckenzie https://github.com/simonmckenzie the command you issued was incorrect. Please try again.

Examples are:

@dotnet-policy-service agree

and

@dotnet-policy-service agree company="your company"

— Reply to this email directly, view it on GitHub https://github.com/dotnet/efcore/pull/33749#issuecomment-2118609186, or unsubscribe https://github.com/notifications/unsubscribe-auth/AEGD3MWBKEUT4SBD65KGC7DZC26GTAVCNFSM6AAAAABH5AZK5GVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDCMJYGYYDSMJYGY . You are receiving this because you were mentioned.Message ID: @.***>

simonmckenzie avatar May 18 '24 02:05 simonmckenzie

Note: this PR is waiting on a design discussion on #33748

roji avatar May 27 '24 06:05 roji

Hi @roji,

Since the parent issue (#33748) has been put into the backlog pending work on provider-level case-sensitivity settings, this PR is now in an uncertain state.

I would argue that since it addresses a known bug (inconsistent case comparison within the BufferedDataReader), it makes sense to still proceed with this fix now. Additionally, its centralisation of the column comparison logic (plus extra tests) should help with the future case-sensitivity work.

If this isn't desired, I guess I should close this PR.

Please tell me what you think.

simonmckenzie avatar Jun 05 '24 20:06 simonmckenzie

Sorry, I forgot a PR was out - should have commented here. Given that we don't intend to break e.g. the SQL Server provider by changing to be case-sensitive, I think fixing the bug definitely still makes sense - one of us will review it soon.

roji avatar Jun 05 '24 20:06 roji

Thanks.

cincuranet avatar Jun 12 '24 12:06 cincuranet