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

Query: Fixes Equals method on SqlParameter class #2044

Merged
merged 4 commits into from
Dec 18, 2020

Conversation

fnbalaji
Copy link
Contributor

@fnbalaji fnbalaji commented Dec 3, 2020

Pull Request Template

Description

SqlParameterEquality relies on == operator on Name and Value. While this is okay for Name since that is explictly typed as a string, it fails for value since that is typed as an object. For an object (even when it is a string) == falls to Object.ReferenceEquals, while the right thing to do is call the Equals method.

The unit tests were passing because they use literal strings, which have the same reference in testing. I have also changed the unit tests.

Type of change

Please delete options that are not relevant.

  • [] Bug fix (non-breaking change which fixes an issue)

Closing issues

Copy link

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

Please follow the required format: "[Internal] Category: (Adds|Fixes|Refactors) Description"

Examples:
Diagnostics: Adds GetElapsedClientLatency to CosmosDiagnostics
PartitionKey: Fixes null reference when using default(PartitionKey)
[v4] Client Encryption: Refactors code to external project
[Internal] Query: Adds code generator for CosmosNumbers for easy additions in the future.

@fnbalaji fnbalaji changed the title Fixes on SqlParameter equality [Internal] Query: Fix for checking equality on SqlParameter Dec 3, 2020
Copy link

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

Please follow the required format: "[Internal] Category: (Adds|Fixes|Refactors) Description"

Examples:
Diagnostics: Adds GetElapsedClientLatency to CosmosDiagnostics
PartitionKey: Fixes null reference when using default(PartitionKey)
[v4] Client Encryption: Refactors code to external project
[Internal] Query: Adds code generator for CosmosNumbers for easy additions in the future.

@fnbalaji fnbalaji changed the title [Internal] Query: Fix for checking equality on SqlParameter [Internal] Query: Fixes Equals method on SqlParameter class Dec 3, 2020
@github-actions github-actions bot dismissed stale reviews from themself December 3, 2020 22:59

All good!

@fnbalaji fnbalaji changed the title [Internal] Query: Fixes Equals method on SqlParameter class Query: Fixes Equals method on SqlParameter class Dec 5, 2020
@j82w j82w changed the title Query: Fixes Equals method on SqlParameter class Query: Fixes Equals method on SqlParameter class Dec 7, 2020
@j82w j82w changed the title Query: Fixes Equals method on SqlParameter class Query: Fixes Equals method on SqlParameter class Dec 7, 2020
@j82w j82w changed the title Query: Fixes Equals method on SqlParameter class Query: Fixes Equals method on SqlParameter Dec 7, 2020
@j82w j82w changed the title Query: Fixes Equals method on SqlParameter Query: Fixes Equals method on SqlParameter class Dec 7, 2020
@@ -79,7 +79,8 @@ public bool Equals(SqlParameter other)
return true;
}

return this.Name == other.Name && this.Value == other.Value;
return this.Name == other.Name
&& other.Value == null ? this.Value == null : other.Value.Equals(this.Value);
Copy link
Contributor

Choose a reason for hiding this comment

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

This still doesn't work for arrays and object, since they don't override .Eqauls() and we need to do a deep equality here.

The long term / proper solution is to compare the JSONs for equivalency.

Copy link
Contributor

@bchong95 bchong95 left a comment

Choose a reason for hiding this comment

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

Approved, but please track the array and object cases.

@j82w j82w merged commit 076fe82 into master Dec 18, 2020
@j82w j82w deleted the users/fnbalaji/SqlParametersEqualityFix branch December 18, 2020 13:00
@ghost
Copy link

ghost commented Dec 15, 2021

Closing due to in-activity, pease feel free to re-open.

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.

4 participants