Optionally validate requests

[cdisselkoen]

Description of changes

Adds Request::new_with_validation() to the public API.

Issue #, if available

Fixes #191

Checklist for requesting a review

The change in this PR is (choose one, and delete the other options):

• A backwards-compatible change requiring a minor version bump to cedar-policy (e.g., addition of a new API).

I confirm that this PR (choose one, and delete the other options):

• Updates the "Unreleased" section of the CHANGELOG with a description of my change (required for major/minor version bumps).

I confirm that cedar-spec (choose one, and delete the other options):

• I'm not sure how my change impacts cedar-spec. (Post your PR anyways, and we'll discuss in the comments.)

Disclaimer

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

[khieta]

Overall changes look good to me, but I'd like to relitigate the decision to add a new Request::new_with_validation API vs. adding an optional schema argument to Request::new. I feel that the latter is more consistent with our choice for the Entities constructors (#360), although I do concede that it is a more invasive change for users.

[khieta]

Another comment I forgot to add: might be useful to add test cases for the new validate_request function.

[cdisselkoen]

Added a bunch of tests (and did some reorg while I was at it). In the process I fixed a (bug?) in entity validation that exists on the 2.4.x release branch.

I'd like to relitigate the decision to add a new Request::new_with_validation API vs. adding an optional schema argument to Request::new.

This PR follows #191, which resulted from discussion in RFC 11. I'm happy to relitigate this, but we should make sure to include the points discussed on 191 and on RFC 11, and to make sure everyone relevant is included in the discussion.

[cdisselkoen]

Recapping the RFC 11 discussion as I see it, along with the above point:

Let's call the solution described in #191, with a separate Request::new_with_validation() constructor, the "two-constructor solution", and the alternative, where we add an optional schema parameter to Request::new(), the "one-constructor-solution".

• The two-constructor solution is back-compatible, while the one-constructor solution breaks existing users.
• @mwhicks1 argues for the two-constructor solution "because I don't want to break existing users, and because I expect that the non-validation path is the common one. People will largely use the validation path for debugging."
• @khieta argues that the one-constructor solution does break existing users, but "the benefit is that this will force them to be aware of the new feature and start validating their requests."
• As precedent for the two-constructor solution, we already have Request::new_with_unknowns() for partial evaluation. We did not break Request::new() when adding partial evaluation. (Note that partial evaluation APIs are still experimental, and we could still change them)
• As precedent for the one-constructor solution, we decided to add an optional schema argument to some Entities methods in Automatically add action entities to Entities; validate entities against schema #360, breaking existing users of those methods.
• @john-h-kastner-aws said at the time "somewhat in favor of making the breaking change"
• I said at the time "Same. It seems like a worthwhile change to force people to adapt to when updating to Cedar 3.x."
• @ChunghaSung's comment on New API for validating requests #191 supports request-validation-by-default, finding the current default (no request validation) to be unintuitive. I interpret this as support of the one-constructor solution.

[mwhicks1]

I could be persuaded that one constructor is Ok, on the basis that old code will not typecheck, and the fix to get it typecheck (include None as the second argument) is easy.

I wonder: Is it easy/possible to build tools that might carry out such a change automatically? We have other APIs we might like to change in ways that are not back-compatible, but are easy to migrate.

[cdisselkoen]

Another point is that, as you can see in the current version of this PR, the two-constructor solution really becomes a four-constructor solution when you consider partial evaluation: we have Request::new(), Request::new_with_unknowns(), Request::new_with_validation(), and Request::new_with_unknowns_and_validation(). This is a little unwieldy IMO. And it definitely doesn't scale to any future features/options we might want to add to Request construction.

[andrewmwells-amazon]

I would vote for the one-constructor solution.

Is it easy/possible to build tools that might carry out such a change automatically?

In Jetbrains products you can do "Change Method Signature" (tested with RustRover). It looks like RustAnalyzer has an issue for this (rust-lang/rust-analyzer#6499) but it isn't done yet.

[andrewmwells-amazon]

nit: I don't think we need to repeat this since it's in the docstring above.

[cdisselkoen]

The docstring above is to document ValidatorSchema::context_type's return invariant. The comment here is explaining how we meet that invariant, namely by discharging it to ValidatorActionId::context_type, which maintains the same invariant on its return value.

[john-h-kastner-aws]

Still in favor of the one-constructor option.

[cdisselkoen]

Seems like the majority are in favor of the one-constructor option, and no one strongly opposed. I updated this PR to take the one-constructor approach.