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

Empty hasOne relation should return null instead of {} when serialized #1839

Merged
merged 4 commits into from May 10, 2018

Conversation

@ricardograca
Copy link
Member

@ricardograca ricardograca commented May 9, 2018

Introduction

When fetching a model that has a hasOne relation and that relation doesn't exist the relation should be null when serializing the model. Currently it's an empty object {}.

Motivation

This makes the hasOne relationship behave like belongsTo which also exhibited this same unexpected behavior until it was changed with PR #1563. All other relation types continue working the same way, so a hasMany without any records is still an empty array [] when serialized.

This should make it easier to determine if a hasOne relation exists or not since after serialization you can just write:

if (model.singleRelation) { ... }

instead of:

if (model.singleRelation && model.singeRelation.id) { ... }

Proposed solution

This changes the eager pairing code to ensure that the relation creation part is skipped if the response from the SELECT query that tries to fetch the related data is empty. The eager pairing would run pushModels() which would always create relation data on the parent model, even when there is no data, resulting in the empty object that was present before this change.

This is a behavior similar to the belongsTo relation, except that that one can be determined much earlier in the eager loading code chain.

Fixes #1771.

Current PR Issues

This is a breaking change, but it shouldn't cause too much trouble since any existing conditions, as in the example above, will still continue working. Other use cases that attempt to add or read properties directly from the serialized output will break.

ricardograca added 3 commits May 9, 2018
- Since pushModels would still pair an empty model, corresponding to the
non-existant relation, with the parent model, the result was that when
serializing the parent model there would be an empty object for the
missing relation instead of the expected null value, like what happens
with belongsTo.
@ricardograca ricardograca added this to To Do in Version 0.14.0 via automation May 9, 2018
@ricardograca ricardograca moved this from To Do to In progress in Version 0.14.0 May 9, 2018
@ricardograca ricardograca merged commit 73b47dc into master May 10, 2018
2 checks passed
2 checks passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
Version 0.14.0 automation moved this from In progress to Done May 10, 2018
@ricardograca ricardograca deleted the rg-empty-hasone branch May 10, 2018
@OlivierKamers
Copy link

@OlivierKamers OlivierKamers commented Sep 11, 2019

Is it correct that the behaviour is to actually omit the relation from the serialised model (ie making it undefined) instead of setting it to null?

@ricardograca
Copy link
Member Author

@ricardograca ricardograca commented Sep 11, 2019

Are you asking if it can be changed to that or are you saying that it's actually undefined currently?

@OlivierKamers
Copy link

@OlivierKamers OlivierKamers commented Sep 11, 2019

I noticed that it's actually undefined currently. So I was wondering if I might be doing something wrong or if it's supposed to behave like that.

@ricardograca
Copy link
Member Author

@ricardograca ricardograca commented Sep 11, 2019

Good point. Looks like it is not even defining the relation key at all. This only matters if you explicitly look for null or check if the key is present, but it should be consistent indeed. Can you open a new issue?

@OlivierKamers
Copy link

@OlivierKamers OlivierKamers commented Sep 12, 2019

Done, thank you for the quick response.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Linked issues

Successfully merging this pull request may close these issues.

2 participants