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 namespace collision for string and CollectionUtils method IsNullOrEmpty #2471

Merged
merged 6 commits into from
Nov 27, 2024

Conversation

abhishekkumams
Copy link
Contributor

@abhishekkumams abhishekkumams commented Nov 20, 2024

Why make this change?

What is this change?

  • MsSqlQueryBuilder class had a check in the Build method where it was using IsNullOrEmpty for Strings but it was refrencing CollectionUtils Class instead
  • Removed Collection.Utilities class from Cli and renamed it in the Core project to EnumerableUtilities to avoid any future namespace collision.
  • Also updated all the references of IsNullOrEmpty

How was this tested?

  • manual tests
  • current existing tests

Notes:
Work Around shared here: AzureAD/azure-activedirectory-identitymodel-extensions-for-dotnet#1722

Copy link
Contributor

@aaronburtle aaronburtle left a comment

Choose a reason for hiding this comment

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

Thanks for fixing this one, I will follow up with another PR that fixes any additional usages.

@aaronburtle
Copy link
Contributor

Actually, after searching the codebase there was only 1 more instance where string was being null checked wrong, so just added it in here, hope that's OK @abhishekkumams

@abhishekkumams
Copy link
Contributor Author

/azp run

@aaronburtle
Copy link
Contributor

/azp run

Copy link
Contributor

@Aniruddh25 Aniruddh25 left a comment

Choose a reason for hiding this comment

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

Need to remove unnecessary CollectionUtils class

@abhishekkumams
Copy link
Contributor Author

abhishekkumams commented Nov 25, 2024

Need to remove unnecessary CollectionUtils class

It's still being used for the reason pointed out by Sean: #2471 (comment)

@abhishekkumams
Copy link
Contributor Author

/azp run

Copy link
Contributor

@sezal98 sezal98 left a comment

Choose a reason for hiding this comment

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

lgtm

@abhishekkumams abhishekkumams dismissed Aniruddh25’s stale review November 27, 2024 10:15

updated as suggested

@abhishekkumams abhishekkumams merged commit 26a694c into main Nov 27, 2024
7 checks passed
@abhishekkumams abhishekkumams deleted the dev/abhishekkuma/fix-namespace-collision branch November 27, 2024 10:16
abhishekkumams added a commit that referenced this pull request Nov 28, 2024
…OrEmpty` (#2471)

## Why make this change?

- Closes #2469 
- There is a namespace collision for the `IsNullOrEmpty` method in the
`MsSqlQueryBuilder` class between String and CollectionUtils class.

## What is this change?

- `MsSqlQueryBuilder` class had a check in the `Build` method where it
was using `IsNullOrEmpty` for Strings but it was refrencing
CollectionUtils Class instead
- Removed `Collection.Utilities` class from Cli and renamed it in the
Core project to `EnumerableUtilities` to avoid any future namespace
collision.
- Also updated all the references of `IsNullOrEmpty`

## How was this tested?

- [x] manual tests
- [x] current existing tests

Notes:
Work Around shared here:
AzureAD/azure-activedirectory-identitymodel-extensions-for-dotnet#1722

---------

Co-authored-by: Aaron Burtle <[email protected]>
Co-authored-by: aaronburtle <[email protected]>
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.

[Bug]: Conflicting namespace for method isNullOrEmpty causing update mutation to break
6 participants