tempest-framework icon indicating copy to clipboard operation
tempest-framework copied to clipboard

`#[Hidden]` model attribute

Open erikaraujo opened this issue 8 months ago โ€ข 10 comments

Description

I believe Tempest needs a Hidden attribute for model properties.

Here's what I believe it should do:

  • When a property is marked as Hidden, it should never be returned by default when querying the models
  • There should be a way to include hidden fields in a query, though (maybe ->include('password', 'some_other_hidden_field')? or maybe just by also specifying it in the ->select() call?)
  • You should still be able to UPDATE hidden fields

Alternatively, it could be named SensitiveField or SensitiveProperty to align with PHP's native SensitiveParameter. Although I don't think they're technically the same thing.

Benefits

It'd be a better way to "protect" certain fields from being leaked by accident via queries.

erikaraujo avatar Apr 25 '25 11:04 erikaraujo

I'd prefer a naming similar to "sensitive field" to better reflect what it actually is

innocenzi avatar Apr 25 '25 12:04 innocenzi

I'd prefer a naming similar to "sensitive field" to better reflect what it actually is

My only "problem" with a name like that is that the native PHP attribute redacts the value - doesn't "remove" it or anything.

Also, #[Hidden] is more generic and more obvious (in my opinion). It also lets users hide fields by default - even if they're not really "sensitive" without causing some name weirdness or whatever.

erikaraujo avatar Apr 25 '25 12:04 erikaraujo

Your last point is a good point actually. But "hidden" is way too generic. If you see it for the first time, you'd wonder what it doesโ€”my first thought would probably be that it's not included in serialization, but your suggested behavior is to skip it by default when querying.

Maybe ExcludeFromQueries?

innocenzi avatar Apr 25 '25 13:04 innocenzi

Yea, I guess ExcludeFromQueries is super clear and not misleading. I like it!


I was thinking, though. TypeORM and MikroORM have a column/property (respectively) decorator.

TypeORM's column decorator lets the user define the following:

  type: 'varchar',
  length: 255,
  nullable: false,
  default: 'user',
  unique: true,
  name: 'user_name',
  select: true,
  update: true,
  insert: true,
  comment: 'Stores the username',

I wonder if this is something worth considering too - maybe you want to "guard" a column and you'd be able to just do #[Column(insert:false, update:false)], but you'd still let it be selected. Or, for passwords, you could do #[Column(select:false)].

It could even be an alternative to MapTo, like #[Column(name: 'my_column')].

I don't know. I'm suggesting that, but I'm honestly not a big fan of it ๐Ÿ˜… .

erikaraujo avatar Apr 25 '25 15:04 erikaraujo

IMHO, I think we need to be very careful to avoid persistence coupling. That doesn't mean that people couldn't represent their persistence schema as a model, but that we build the functionality in a way that heavily supports persistence ignorance.

For that reason, I'd suggest that we avoid things like the #[Column] attributes and find creative ways to support other practices with the same standard of DX.

This is (one of) the biggest areas Tempest has to shine in contrast to other solutions out there today.

aidan-casey avatar Apr 25 '25 16:04 aidan-casey

As a side note, that may also make "sensitive data" more of a dto/serialization/view/resource concern as well if we really think about it.

aidan-casey avatar Apr 25 '25 16:04 aidan-casey

IMHO, I think we need to be very careful to avoid persistence coupling. That doesn't mean that people couldn't represent their persistence schema as a model, but that we build the functionality in a way that heavily supports persistence ignorance.

For that reason, I'd suggest that we avoid things like the #[Column] attributes and find creative ways to support other practices with the same standard of DX.

This is (one of) the biggest areas Tempest has to shine in contrast to other solutions out there today.

I do agree - to some extent. Some things cannot be avoided if the framework wants to facilitate certain things. I think ideally the framework should support persistence ignorance, but not necessarily avoid including certain things because of it.

For example, at some point, Tempest will have to have a way to get primary keys in order to fetch relationships (currently it relies on the model having an id column - but that's gonna go away). Also, it does have a Table attribute already.

Say, for example, Tempest introduces ExcludeFromQueries - that's very much NOT "persistence ignorance" - but people can still use the framework without ever using it by manually selecting columns without letting the framework infer them. The same way people can also avoid using the Table attribute. On the other hand, those who would like to "tie" their models to being a DB model can leverage those helpers.

On the other hand, I do agree that, whenever possible, it'd be better to use more "generic" terms that would work for both scenarios. I just think we should be careful as to not end up with confusing things that don't really result in what people would expect them to.

Like #[SensitiveData] or similar may lead people to expect the value to be queried - but never logged or showed in an exception, like the native #[SensitiveParameter] works.

Either way, I'm actually not a fan of the #[Column] attribute myself - but I wanted to throw that out there just in case.

erikaraujo avatar Apr 25 '25 18:04 erikaraujo

After giving it some thought, I back to preferring #[Hidden]. We can have it work for both the "DB" situation as well as not include it in serialization by default.

Would there be an issue with that approach? Basically - if it's a DB query, not even fetch it - if the value is already in the model, don't serialize it.

I don't know, I'm afraid it could still be confusing, but documentation will be there to help.

erikaraujo avatar Apr 26 '25 12:04 erikaraujo

I think #[Hidden] makes the most sense and could be generically supported by the mapper as well. I agree with Aidan that we should steer away from database-specific attributes as much as possible.

brendt avatar Apr 28 '25 08:04 brendt

I'm adding post-1.0 milestone, but someone is free to pick it up before of course

brendt avatar Apr 28 '25 08:04 brendt