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

update hyperlinkrelated to gracefully support nullable / non-required sqla relations #72

Closed

Conversation

@feigner
Copy link

commented Feb 18, 2018

I have nullable relations on my SQLAlchemy models that blow up when deserializing HyperlinkRelated. Patch allows for graceful failure when base field is not-required.

Similar to the issue listed here: #18

@antgel

This comment has been minimized.

Copy link

commented Feb 9, 2019

@feigner Lots of interest in this according to various Issues (e.g. #18 and #68). Perhaps you can rebase against latest master, then @sloria would you be able to merge it?

@sloria

This comment has been minimized.

Copy link
Member

commented Feb 11, 2019

Sure. @feigner Would you mind getting this up to date with dev (rebase or merge are fine)?

@feigner

This comment has been minimized.

Copy link
Author

commented Feb 11, 2019

@feigner feigner force-pushed the feigner:update/nullable_url_for_dump branch 3 times, most recently from 6760de8 to 75ced3c Feb 12, 2019

@feigner feigner force-pushed the feigner:update/nullable_url_for_dump branch from 75ced3c to 8db469e Feb 12, 2019

@feigner

This comment has been minimized.

Copy link
Author

commented Feb 12, 2019

Allllrighty @sloria & @antgel, just rebased against dev and updated accordingly. Looks like tests are all now passing 👍

@sloria

This comment has been minimized.

Copy link
Member

commented Feb 13, 2019

@feigner Thanks for getting this up to date. I'm unsure how this will affect partial loading. Can you add a test for it?

@feigner

This comment has been minimized.

Copy link
Author

commented Feb 19, 2019

Happy to write some more tests. I've read the linked docs, but I'm not entirely sure what you'd like me to verify. Can you give me an example use case of what you'd like to see?

@sloria

This comment has been minimized.

Copy link
Member

commented Feb 25, 2019

Oh, didn't realize this only changes serialization behavior, so partial loading isn't relevant here.

However, I'm not sure about the if not self.required check. marshmallow does not validate on serialization (by design), so _serialize shouldn't do any "required" validation.

I think the most consistent behavior would be to serialize None to None, as the other fields do.

sloria added a commit that referenced this pull request Mar 10, 2019

HyperlinkRelated serializes None to None
For consistency with other fields

close #72
@sloria

This comment has been minimized.

Copy link
Member

commented Mar 10, 2019

Upon looking this over again, I think this isn't the right approach for the reason I mentioned above. required and missing should only be used in deserialization. The proper fix is actually simpler--just serialize None to None. I've done this in #125.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.