Skip to content

Conversation

@ayush3797
Copy link
Contributor

@ayush3797 ayush3797 commented Mar 30, 2022

Why is this change being made?

The code changes are meant to add support for assigning NULL value to columns for REST operations like POST,PUT,PATCH.

What changed?

  1. Added checks which prevent assignment of NULL value to non-nullable columns in different REST operations.2.

  2. Removed the code which used to throw an error whenever a NULL value was assigned to a column in the request body irrespective of whether the column was nullable or not.

How was this validated?

  1. Added positive test cases for INSERT/PUT/PATCH operations having NULL value assigned to the columns that were nullable.

  2. Added negative test cases for INSERT/PUT/PATCH operations having NULL value assigned to the columns that were non-nullable.

Copy link
Contributor

@seantleonard seantleonard left a comment

Choose a reason for hiding this comment

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

Some comments about breaking out Nullable Tests into their own test methods.

Copy link
Collaborator

@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.

Waiting on making function arguments nullable

@Aniruddh25 Aniruddh25 changed the title Support for null column values in request body added REST Support for null column values in request body added Apr 5, 2022
@Aniruddh25 Aniruddh25 added this to the M1 milestone Apr 7, 2022
@Aniruddh25 Aniruddh25 linked an issue Apr 7, 2022 that may be closed by this pull request
Copy link
Collaborator

@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.

Please resolve all comments. If you think you have resolved them, mark them as such - so reviewers can verify and close.

@gledis69
Copy link
Contributor

gledis69 commented Apr 15, 2022

We should be careful with using IsNullOrWhitespace in the request validator, since some conditionals use that to determine invalid column values, but it will raise errors for empty strings which are be valid input for text columns.

They should be switched to null checks.

@Aniruddh25 Aniruddh25 removed this from the M1 milestone Apr 16, 2022
@ayush3797
Copy link
Contributor Author

We should be careful with using IsNullOrWhitespace in the request validator, since some conditionals use that to determine invalid column values, but it will raise errors for empty strings which are be valid input for text columns.

They should be switched to null checks.

So empty string i.e. "" is a valid value ? I thought its invalid.

@Aniruddh25
Copy link
Collaborator

We should be careful with using IsNullOrWhitespace in the request validator, since some conditionals use that to determine invalid column values, but it will raise errors for empty strings which are be valid input for text columns.
They should be switched to null checks.

So empty string i.e. "" is a valid value ? I thought its invalid.

Good point @gledis69, empty or white space are VALID inputs to string columns.

@ayush3797
Copy link
Contributor Author

@gledis69 , your concern has been addressed.

Copy link
Collaborator

@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 test cases for empty string

{
if (upsertRequestCtx.FieldValuePairsInBody[column.Key] == null ||
string.IsNullOrWhiteSpace(upsertRequestCtx.FieldValuePairsInBody[column.Key].ToString()))
if (upsertRequestCtx.FieldValuePairsInBody[column.Key] == null)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do we have a test case for the null or white space field value? If not, could you please add one, if not added already...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We do have for null values. Want me to add for white space field value?

Copy link
Collaborator

Choose a reason for hiding this comment

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

yes, white space or empty string please.

Copy link
Collaborator

@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.

LGTM after resolving merge conflicts and adding test for empty string.

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.

Add Nullable Type Support for REST

4 participants