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 logic related to NestedModelField and fix several bugs #180

Conversation

ADR-007
Copy link
Collaborator

@ADR-007 ADR-007 commented Feb 11, 2023

  • Allow NestedModelField in ListField (any level. E.g.: [{a: {b: [{c: {d: 'e'}}]}}])
  • Allow update/merge with None (bugfix?)
  • Allow nested fields as model instances in __init__ method (bugfix? e.g. MyModel(nested=MyNested()))
  • Allow update/merge with None in a deeply nested field (bugfix?)
  • Wrap errors to add field path to the error message (e.g.: ... "a.b[0].c.d": field is required). Most likely, it will not break backward compatibility because there wasn't a base exception class, so most likely no one used particle exception in except statement
  • Fix model.update (bug)
  • Fix memory leak
  • Set Meta.ignore_none_field=False by default. Most likely, it will not brake back-compatibility because True value was used to hide bugs in update. But... I think we can ignore None on create but not on update 🤔. It is not really clear how it should work. It probably should be "ignore default None, but save if it was set manually"
  • Refactor some related code:
    • Add base exception class for model exceptions
    • Remove unused code
    • Remove conditions used to hide/resolve bugs for some cases
    • Move all the serialization/deserialization logic to Model. So, now there is only one place for it, which allows handling nested fields in a more convenient way, and overrides this behavior in NeatedModelField.get_value method.

Do I have a chance to have this merged, or is it too big one? Not sure how to do it in smaller PRs btw.
I would like to use this code in a project I'm currently working on. So, I'll test it on real data as soon I'll know I have a chance to have it merged :)

- Allow NestedModelField in ListField
- Allow update/merge with None
- Allow update/merge with None nested field
- Wrap errors to add error.field path
- Fix model.update
- Fix memory leak
- Set Meta.ignore_none_field=False
@ADR-007 ADR-007 changed the title WIP: Update logic related to NestedModelField and fix bunch of bugs WIP: Update logic related to NestedModelField and fix several bugs Feb 11, 2023
@ADR-007 ADR-007 changed the title WIP: Update logic related to NestedModelField and fix several bugs Update logic related to NestedModelField and fix several bugs Feb 13, 2023
@ADR-007
Copy link
Collaborator Author

ADR-007 commented Feb 13, 2023

There is a couple of more things I want to add to the project, but after this PR :)

@AxeemHaider AxeemHaider merged commit 33cf93c into octabyte-io:master Feb 13, 2023
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.

2 participants