Property validation should allow for cross-property validation; lazy property validation should always be forced at converge time before running the action

[atheiman]

I want to be able to validate properties of custom resources using the callbacks validator as described in the docs with more complexity:

• Validate based on the value of another property
• Validate based on the action of the resource

I quickly looked through the validator code and I think there are two possible approaches to this:

1. Allow the callback lambdas to also receive the resource object with access to other properties and the action:

property :a, String
property :b, String, callbacks: {
  'required if property `a` is unset' => lambda { |property, resource|
    if resource.properties.a.nil?
      # if property is nil, return false which raises validation error
      !property.nil?
    end
  },
  'required for `:configure` action' => lambda { |property, resource|
    if resource.action == :configure
      # if property is nil, return false which raises validation error
      !property.nil? 
    else
      true
    end
  }
}

2. Allow the callback lambdas to also receive the action, and introduce a required_with validator that requires at least one of the properties to be set:

property :a, String, required_with: [:b]
property :b, String, required_with: [:a], callbacks: {
  'required for `:configure` action' => lambda { |property, action|
    if action == :configure
      # if property is nil, return false which raises validation error
      !property.nil? 
    else
      true
    end
  }
}

[lamont-granquist]

can you solve this problem a lot easier by just writing an action_helper like def assert_properties_valid! and calling that in all your actions or all the appropriate actions?

the only real utility to doing property validation in the resource is to catch errors at compile time. if you're requiring an action and doing validation between properties, then you're already going to have to lazy validation to converge time (when you have that information accurately available), and might as well do it a lot more directly in the action class itself.

[coderanger]

This kind of thing usually belongs in #after_created, so there is a defined life-cycle point for the multi-property check. Otherwise you can get weird issues with mutation-y objects.

[lamont-granquist]

even in after_created its a bit weird because of possible chef-rewind-ey things. if you've got complicated validation based on multiple properties plus the action then in core chef we usually put that in define_resource_requirements which is morally equivalent to just running a helper inside of the action.

[atheiman]

Yes what I’ve been doing so far is an action helper method like you’ve described. If youre not interested in implementing this and think the helper is a better approach thats fine with me.

[lamont-granquist]

Yeah, the action helper approach is much clearer behavior on both sides (both for the core chef author of the properties code, and for the writer of custom resources).

A meta-question is if you went down this road due to being excessively paranoid about input validation. We used to do a lot of regex checking against e.g. usernames in core chef, but when you go down that road and start worrying about UTF-8 and Latin-1 and all the variations, and strange hacks people use for directory services, then the result was that we just banned a few always-bad characters like ':' on unix-related usernames, and then rely on the underlying O/S useradd to barf for us and catch the ShellOut exception. Trying to precisely replicate the validation that different platforms used across all the different O/S distros and versions was just going to be a neverending source of bugs and pain -- so instead of having Chef do validation we allowed the distro to be the source of truth...

Just something to consider...

[lamont-granquist]

Doing this as validation in after_created always breaks lazy values. So if property A depends on validating property B then if you validate them in after_created you force A and B to un-lazy at compile-time. That always generates bug reports by people who lazy absolutely everything.

[lamont-granquist]

I think we also have issues around if you lazy a required value and such as well. We should really run through all the properties and force validation at converge time before running the action to make sure that all lazy validation is forced to be run.

[lamont-granquist]

We have the check_resource_semantics! callback on the provider which should be used for this kind of lazy cross resource validation. The lazy required validation is being done in #9688. We might look at implementing cross-property-lazy-aware-validation directly in properties, but its probably going to be driven by cleanup of our existing resource code at some point. For now all that code should be moved to check_resource_semantics! in the provider.

[lock]

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.