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

Properly handle payloads where properties are set to null or empty string in requests and response #243

Closed
MIchaelMainer opened this issue Jun 17, 2021 · 3 comments · Fixed by #481
Assignees
Labels
Csharp Pull requests that update .net code fixed Java type:bug A broken experience TypeScript Pull requests that update Javascript code
Milestone

Comments

@MIchaelMainer
Copy link
Member

MIchaelMainer commented Jun 17, 2021

This to be considered for serialization and backing store.

Request scenarios

  • PATCH a property to null - backing store should track change to null and the serializer should serialize null for properties changed to null.

Response scenarios

  • Delta query where a property has been set null. This set null needs to be differentiated upon deserialization from the default value of the property so that consumers can be notified of the change to null.
  • Change notifications, maybe impacted if they notify null in payload.
  • Selecting and expanding properties - if a selected property is null in a response, should we differentiate the deserialized null from the default null value when instantiating an object? What about when we make a call without select and we get back the default object properties? There is subtle difference between the default value of a property on the client is null when instantiating an object, versus the server verifying that the property is actually null at the time of the fetch. The difference is about having certainty about the null value of the property. If there was a time component to the backing store, we could have confidence based on the timestamp of the property being set to null (or an actual value).

#223 Backing store PR

AB#9945

@baywet baywet self-assigned this Jul 5, 2021
@baywet baywet added the type:bug A broken experience label Jul 5, 2021
@baywet baywet modified the milestones: Beta, Alpha Jul 5, 2021
@baywet
Copy link
Member

baywet commented Jul 5, 2021

Thanks for the feedback @MIchaelMainer.
I believe this is a case we can only fix when people are using backing stores because we don't want to have to generate properties with wrappers to tell us the difference between nil and null.

The proper way to fix that would probably to add a GetNullAndChangedPropertiesNames that would return the names of the properties that have been changed to null.
Then in the backing store callback we could call that method, and call the serialization writer to write all those properties to null in the payload.
This would keep separation of concerns where the serialization writer doesn't know about the backing store and the backing store doesn't know about the serialization format.

@baywet
Copy link
Member

baywet commented Jul 5, 2021

also to answer your third point a bit more thoroughly, :

  • assuming you select=id,displayName -> user.backingStore.enumerate -> won't return givenName
  • assuming you select=id,displayName,givenName, and givenName is returned in the payload -> user.backingStore.enumerate with return given name with the value it's set to (including null).

@baywet baywet modified the milestones: Alpha, Beta Jul 16, 2021
@baywet baywet added Csharp Pull requests that update .net code Java TypeScript Pull requests that update Javascript code labels Jul 21, 2021
@baywet baywet added the fixed label Aug 13, 2021
@baywet
Copy link
Member

baywet commented Aug 13, 2021

thanks again for reporting this @MIchaelMainer this will be fixed by #481

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Csharp Pull requests that update .net code fixed Java type:bug A broken experience TypeScript Pull requests that update Javascript code
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants