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

4.0: Incosistencies of public? for fields and for arguments and its effect on accept #1646

Open
vonagam opened this issue Dec 5, 2024 · 6 comments
Labels
enhancement New feature or request

Comments

@vonagam
Copy link
Contributor

vonagam commented Dec 5, 2024

There are two things that differ for public? option between fields (attributes/relationships/aggregates/calculations) and arguments - meaning and defaults.

Meaning: for fields right now it means "can it be read over public interfaces", for arguments - "can it be written over public interfaces".

Defaults: for fields it defaults to false and they need to opt-in to appear in an interface, while for arguments it is false and they need to opt-out from being public (like in Ash 2).

Those discrepancies also manifest in accept list creating all attribute arguments as public even if an attribute itself is not.

So it ends up being that:
Writing a private argument - not ok.
Writing a private attribute - ok.

I am of opinion that it is better to consolidate meanings and defaults in next version of Ash.

For meanings it should mean "can it be read or written over public interface"/"can it be present in public interface". Because of the same meaning accept generated arguments will be able to inherit the option from its attribute. And to allow exposure of private attributes for writing in an action an option similar to require_attributes can/should be added.

For defaults arguments should match all other places and default to being private with publicity being opt-in.

@vonagam vonagam added enhancement New feature or request needs review labels Dec 5, 2024
@zachdaniel zachdaniel changed the title Incosistencies of public? for fields and for arguments and its effect on accept 4.0: Incosistencies of public? for fields and for arguments and its effect on accept Dec 5, 2024
@frankdugan3
Copy link
Contributor

FWIW, if this is addressed in the future, I prefer Zach's suggestion to simply rename for better clarity:

argument :foo, :bar, internal?: true 

Because for arguments, public? is actually about visibility, not writeability.

@vonagam
Copy link
Contributor Author

vonagam commented Dec 5, 2024

Introducing internal? (returning private? with different name) might be a solution in terms of separating expectations but it has downside of creating additional concepts.

I personally prefer things to be simpler and matching as in:
public? - controls presence in public interfaces and by default false. That's it.

What is the downside?

Zach mentioned that the decision to keep arguments public by default was based on fact that most (99%) of them are public and it would be awkward to write for each argument. For me when I heard about public? change in Ash 3 I was concerned that it would become annoying (as even if not 99% but I'm sure that above 50% of all fields are public, at least in codebases that I've seen). But it was ok and writing public?: true for arguments (when I did not know that they have separate default) was not different from other places in terms of awkwardness.

@frankdugan3
Copy link
Contributor

frankdugan3 commented Dec 5, 2024

The decision regarding public? for attributes/relationships/calculations/aggregates was more about safety, which I don't think applies the same way for arguments.

@vonagam
Copy link
Contributor Author

vonagam commented Dec 5, 2024

Exposing an argument to due to implicit visibility defaults which should not have been exposed is a valid safety concern.

And in that regard it even worse than it was in Ash 2 as in Ash 2 defaults for private? options were consistent but here you need to know that only for arguments it is different.

(And on percentages - looking at realword repository (which is not representative of a real world but still) all custom attributes except one are marked with public?: true.)

@frankdugan3
Copy link
Contributor

What I meant is that attributes are applicable to the entire resource. Therefore, they are a safety concern because an attribute could be added later that is inherited by all actions. Arguments on the other hand, are added directly on each action, so behavior is very clear and limited in scope.

@vonagam
Copy link
Contributor Author

vonagam commented Dec 5, 2024

limited in scope

I agree. (But even limited vulnerability is still a vulnerability.)

very clear

I disagree. (Mismatch in meaning and defaults with all other places.)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

3 participants