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

Fix fillable and guarded attribute behavior on mass-assignment #830

Merged

Conversation

stoutput
Copy link
Contributor

@stoutput stoutput commented Nov 17, 2022

Previously, the __fillable__ and __guarded__ attributes were only partially supported, and only on create.

This PR:

  • adds full support for the __fillable__ and __guarded__ attributes on create, bulk_create, and update
  • corrects the default value of __guarded__ to be []
    • [] means no attributes are guarded against mass assignment, thus allowing any field to be set by update/create
    • ['*'], by contrast, means that all fields on the model are guarded against mass assignment and cannot be set with update/create
    • It's possible that you may not want fields filtered on QueryBuilder.create() since this is a more "direct" method call and would still allow for creation if all the fields are excluded by fillable/guarded. If so let me know and I can remove this behavior
  • now raises if both __fillable__ and __guarded__ are defined on the base model. This mirrors Orator's behavior.
  • fleshes out full support of optional casting of values during create/bulk_create/update
  • cleans up requirements.txt to only install runtime dependencies. Dev dependencies have been moved into requirements.dev and the makefile adjusted
  • updates black due to a bug with one of its dependencies, click (in order to run formatting for this PR)
  • cleans up a whole lot of code along the way

Note: I still can't run tests locally in a fresh environment, which makes contributing well-tested code somewhat difficult. Was getting the error about a missing ORM config file.

Update: I was able to get most tests passing locally by creating a database config file. Its path gets set during testing only.

@stoutput
Copy link
Contributor Author

@josephmancuso This is ready for review. Sorry, I know it's a big one.

@BSN4
Copy link
Contributor

BSN4 commented Dec 6, 2022

looks nice, so many code got refactored though need to be reviewed and tested for sure

@stoutput
Copy link
Contributor Author

@josephmancuso Think you can review this one soon? Thanks!

@josephmancuso
Copy link
Member

Yes my apologies ill get to this. Its on my TODO

@zombeer
Copy link

zombeer commented Dec 21, 2022

I'm also waiting for this fix

src/masoniteorm/config.py Outdated Show resolved Hide resolved
@stoutput
Copy link
Contributor Author

stoutput commented Jan 2, 2023

Ready for a second pass @josephmancuso

@stoutput stoutput force-pushed the fix-fillable-guarded-behavior branch from 4138be2 to 6790ec3 Compare January 2, 2023 17:55
Copy link
Member

@josephmancuso josephmancuso left a comment

Choose a reason for hiding this comment

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

Just want to understand the CI changes you made more

@@ -0,0 +1,2 @@
use asdf
layout python
Copy link
Member

Choose a reason for hiding this comment

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

Whats this file for?

@@ -58,7 +58,7 @@ jobs:
python orm migrate --connection mysql
make test
lint:
runs-on: ubuntu-latest
runs-on: ubuntu-20.04
name: Lint
Copy link
Member

Choose a reason for hiding this comment

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

Whats the point of pegging this to a version instead of always testing against the latest ubuntu version?

Copy link
Member

Choose a reason for hiding this comment

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

If we always use the latest we can catch issues as soon as the latest version is updated on that container. If we always peg to 20.04 we'll know it works on ubuntu 20.04 but never anything newer than that

@stoutput
Copy link
Contributor Author

@josephmancuso replied to your comments – let me know if you have any other questions!

@stoutput
Copy link
Contributor Author

Hey @josephmancuso I'm sure you're super busy but it'd be great if we didn't let this one languish, there are a few people waiting on this fix. If you want, I'd be open to taking on a co-maintainer role of the ORM repo while you're busy.

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.

None yet

4 participants