spring-security Fix/datasource validation

This PR removes a redundant conditional check in the doParse method of the specified class. The condition dataSource != null is always evaluated as true because the getAttribute method always returns a value. By removing this redundant check, the code becomes more concise and easier to read while maintaining the error handling logic when necessary. This change aligns with best practices for code simplification and improves code quality.

Mar 13 '24 14:03 sheriumair

@sheriumair Please sign the Contributor License Agreement!

Click here to manually synchronize the status of this Pull Request.

See the FAQ for frequently asked questions.

Mar 13 '24 14:03 pivotal-cla

@sheriumair Thank you for signing the Contributor License Agreement!

Mar 13 '24 15:03 pivotal-cla

@sheriumair thanks for the PR!

It's possible the original code was incorrect (this code goes back to 2007), but the null check appears to have been intended to handle the situation where the data-source-ref attribute is missing.

while maintaining the error handling logic when necessary.

I'm not sure the fix is correct. Can you please update the PR to correctly fix the issue and add a test asserting the behavior you are fixing (e.g. an XML with missing data-source-ref)?

Mar 14 '24 21:03 sjohnr

Hey @sjohnr ! Thanks for pointing out this issue in my PR. dataSource variable can be empty but not null. So if it is empty then we have to handle that case. I will correct this issue in my upcoming PR and write a test where data-source-ref is missing.

Mar 15 '24 07:03 sheriumair

Hey @sjohnr! Can you kindly review the PR, please?

Mar 23 '24 01:03 sheriumair

Hey @sjohnr. Thank you for your review. I have changed to hasText and added another test when data-source-ref attribute is entirely missing.

Mar 25 '24 19:03 sheriumair

Hey @sjohnr ! Apologies for the oversight, I committed the code without running the Checkstyle analysis. I have now corrected the code to adhere to the coding standards and have ensured that it passes the Checkstyle checks. Please review the updated PR. Thank you for your understanding.

Mar 26 '24 20:03 sheriumair

Once you are finished with the above updates, would you mind please squashing your commits?

Mar 26 '24 21:03 sjohnr

Hey @sjohnr! I have squashed my commits and changed the XML to multi line

Mar 27 '24 18:03 sheriumair

Hi @sheriumair! Looks like there's still two commits? I think there should only be one.

Mar 27 '24 21:03 sjohnr

Hey @sjohnr . I hope my latest PR solves all the issues.

Mar 28 '24 15:03 sheriumair

Hey @sjohnr! Is there anything else that is left on my side?

Apr 03 '24 17:04 sheriumair

@sheriumair I don't believe so. I will work on getting this merged tomorrow. If anything is remaining I will apply a polish commit. Thanks for your contribution!

Apr 03 '24 21:04 sjohnr

Thanks @sheriumair! This has now been merged as 33ebd5405a79d72f8b2b60c4b3c5fd88dcd5ef7f with polish commit 39dbd24dcbd5ce7ab21b5801ee6c4cc165e2b6cb.

Apr 04 '24 19:04 sjohnr