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

Add new property required indicator to RegisterProperty #4351

Open
StefanOssendorf opened this issue Nov 27, 2024 · 5 comments
Open

Add new property required indicator to RegisterProperty #4351

StefanOssendorf opened this issue Nov 27, 2024 · 5 comments

Comments

@StefanOssendorf
Copy link
Contributor

Would it be useful to have a bool (or enum?) to declare "This property must be set (!= null) after any data portal operation".
So that the framework would check all marked properties if they have value != null. If a violation is deteced it'll raise an RequiredPropertyNotSetException to provide immediate feedback for any developer.

Describe the solution you'd like
Tbh I'm not a fan of using yet another bool but something like RegisterProperty<Foo>(nameof(MyFoo), isRequired: true); or a enum like RegisterProperty<Foo>(nameof(MyFoo), options: PropertyOptions.Required); where PropertyOptions is a flag so we can use it in the future for more settings?

Describe alternatives you've considered
The developer is obligated to have some kind of guard method which must be added to all fetch, create, update, ... methods.

@kant2002
Copy link
Contributor

The main question what are the other options in the enum? It seems that passing enum is optimization for some future which may never come. Maybe you hav e some concrete ideas of what other enum fields would be

@rockfordlhotka
Copy link
Member

We already have overloads that accept a flags enum to indicate lazy loading. My thought was to add a new flag to that existing enum - no extra overloads of RegisterProperty necessary.

@rockfordlhotka
Copy link
Member

If we can detect, at runtime, that a property/field is required then we should be able to use that metadata all the way down the call chain into the set/load property methods to throw if they are provided a null value. Then within the call chain we can safely assume that any required property will never have a null value.

Such properties also can never be allowed to go to a default value, because default would be null, so they have to be initialized to something as part of the create/fetch operations.

@StefanOssendorf
Copy link
Contributor Author

The main question what are the other options in the enum? It seems that passing enum is optimization for some future which may never come. Maybe you hav e some concrete ideas of what other enum fields would be

The main point was to have an easier time in the future to add new option values instead of adding more booleans or changing from a bool to an enum.
But as rocky stated we already have that enum. I totally forgot that!

@StefanOssendorf
Copy link
Contributor Author

StefanOssendorf commented Nov 28, 2024

If we can detect, at runtime, that a property/field is required then we should be able to use that metadata all the way down the call chain into the set/load property methods to throw if they are provided a null value. Then within the call chain we can safely assume that any required property will never have a null value.

Such properties also can never be allowed to go to a default value, because default would be null, so they have to be initialized to something as part of the create/fetch operations.

That is my intention. We have access (or can make it available) to the FieldManager during fetch, create, update, etc to check exactly that. Like we changed in #3776 to wait for non-busy.

There will only be these situations where these properties can (and will) be null

  • During a fetch
  • During a create

All other methods (update, execute, delete) will have these properties set.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants