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

__fillable__ and __guarded__ are mutually exclusive... why? #846

Closed
circulon opened this issue Mar 16, 2023 · 4 comments
Closed

__fillable__ and __guarded__ are mutually exclusive... why? #846

circulon opened this issue Mar 16, 2023 · 4 comments
Labels
bug An existing feature is not working as intended

Comments

@circulon
Copy link
Contributor

Describe the bug
Currectly (in v 2.19.1) cannot specify both fillable and guarded in a model together.
The documentation here: https://orm.masoniteproject.com/models#mass-assignment
implies (to me) that its ok.

if used together in the same model the following exception is raised when the model is instantiated.
SomeModel must specify either __fillable__ or __guarded__ properties, but not both.

This kind of defeats the purpose of being able to filter fields explicitly for different scenarios where they should or should not be mass assigned.

Additionally the Model.filter_mass_assignment() method actually uses both these attributes

@classmethod
def filter_mass_assignment(cls, dictionary: Dict[str, Any]) -> Dict[str, Any]:
    """
     Filters the provided dictionary in preparation for a mass-assignment operation
     Wrapper around filter_fillable() & filter_guarded(). Passed dictionary is not mutated.
    """
    return cls.filter_guarded(cls.filter_fillable(dictionary))

So my question is, is this a design choice or is this error a false negative bug where the idea could have been possibly to check a column name cannot be in both __fillable__ and __guarded__ at the same time?

The Scenario where both these would be used is during a create followed by an update or explicit .save()
so the guarded columns could not be accidently changed usinf mass assignment.

To Reproduce
Create a model that contains fillable and guarded

class Company(Model):
    __table__ = "company"

    __guarded__ = [
        "customer_ref",
    ]

    __fillable__ = [
        "name",
        "email",
        "phone",
        "active",
    ]

Instantiate the model

model = Company()
# raises "Company must specify either __fillable__ or __guarded__ properties, but not both."

Expected behavior
Expect the model to be instantiated.

Desktop (please complete the following information):

  • OS: Mac OSX
  • Version 12

What database are you using?

  • Masonite ORM 2.19.1
@circulon circulon added the bug An existing feature is not working as intended label Mar 16, 2023
@circulon
Copy link
Contributor Author

Looks like this came in as part of #830
"now raises if both fillable and guarded are defined on the base model. This mirrors Orator's behavior."
commit : da1871b

Though looking throught the Orator docs and current code base I cannot seem to find any reference to these attributes being mutually exclusive?

@stoutput can you please provide an explanation?
I found this in the Orator docs:
You can also use the create method to save a model in a single line, but you will need to specify either the __fillable__ or __guarded__ property on the model since all models are protected against mass-assigment by default
This implies you must at least provide a stipulation of columns to be ignored or included when doing mass assignments.
It does not say you cannot use both.

Happy to discuss further to get better understanding.

@stoutput
Copy link
Contributor

Hey @circulon! There's really no need to specify both. One is a blocklist, one is an allowlist. From the docs: The __guarded__ is the inverse and acts as “blacklist”. By specifying __fillable__ you are saying only those fields should be allowed to be set during a create or update - why would you also specify fields in __guarded__ rather than just omitting them from __fillable__? The same question applies vice-versa. I would be curious to see a valid real-world use-case if you can provide one.

The reason filter_mass_assignment filters both is because one or the other may be set, and rather than using conditionals (i.e. if (cls.getattr('__fillable__', None))), we just call through to apply filtering if the attribute is not empty.

@circulon
Copy link
Contributor Author

@stoutput
Yeah I see what you mean and understand that only one or the other is required for the exclude/include to be functional.

I was considering the design / description of the Model itself so apologies for the confusion.
By that I mean defining explicitly what is allowed and what is not, there is no ambiguity about which fields are available (automatically included/excluded) for which context ie mass assignment or single assignment.
But this is just my opinion on an aspect of Model design.

So it looks like the docs need a little clarification added around this... I will have a look at that later.

Happy to discuss further if you like.

@josephmancuso
Copy link
Member

i think this is resolved now from he last reply from @stoutput which makes sense to me

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug An existing feature is not working as intended
Projects
None yet
Development

No branches or pull requests

3 participants