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.

Sign up for GitHub

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

Jump to bottom

Smithy Rules engine #1356

Merged

syall merged 44 commits into smithy-lang:main from skmcgrail:rules-engine

Oct 10, 2022

Contributor

skmcgrail commented Aug 17, 2022 •

edited

Loading

Implementation of the Smithy Rules Engine Language

skmcgrail force-pushed the rules-engine branch 2 times, most recently from 85cfd83 to 84ebd09 Compare

August 18, 2022 23:52

skmcgrail marked this pull request as ready for review

August 28, 2022 19:18

skmcgrail requested a review from a team as a code owner

August 28, 2022 19:18

This was referenced Sep 1, 2022

feat(endpoints)!: integrate endpoints 2.0 smithy-lang/smithy-swift#433

Merged

feat(endpoints)!: integrate endpoints 2.0 awslabs/aws-sdk-swift#607

Merged

skmcgrail force-pushed the rules-engine branch from 59861f7 to 9919d96 Compare

September 20, 2022 03:00

skmcgrail and others added 23 commits

September 29, 2022 17:20


          Rules Engine Initial Port

5f52c06


          Incorporate Latest Rules Engine Commits

b5172cf

eb43dc73...c92510ca


          Fix several cases where serializers were missing and add round-trip t…

f5160cc

…ests (#1)


          Change namespace from reterminus to language

0194b09


          Move resources to correct classpath locations

97838df


          Mark unstable, make classes final that can be

fbfcfdc


          Add UseGlobalEndpoint builtins for STS/S3

521a9e8


          Make UseGlobalEndpoints default to false.

c8a63f7


          Prune synthesizer components


          Update the PartitionFn to fallback to the AWS partition

0ef91c0


          Add more tests of hostLabel standard library function

da17266


          Add disable MRAP builtin

a87b3ae


          constrain substring to the set of ascii characters

c4d5585


          fix doc comment

33f6470


          Integrate Smithy Rules Engine Traits (#8)

cc31c46

* smithy-rules-engine trait fixes
* Integrate of smithy-rules-engine traits


          Fixed additional trait issues (#9)

e220145


          Fix checkstyle

73df2b6


          Windows fixes

a0efefb


          Removed endpoints.json (#10)

ebad2ef


          add partition names to output (#11)

7e884b5


          Rename language to syntax

c0a84e9


          Add aws-iso global regions.

fe5a70e


          Fix tests to pass latest partitions.json (#13)

635104c

alextwoods and others added 17 commits

September 29, 2022 17:20


           Add S3Control builtin + EndpointRuleset toBuilder (#14)

68dc7be


          Fix addRules -> rules usage

5768c7f


          MandatorySourceLocation cleanups

cd15998


          Lots of refactors + extract function registry

afcb970


          refactor booleanequals and string equals

f29c8d5


          Fixups

3fe3e38


          More cleanups: good by eval trait

560f96b


          Delete FnVisitor & address CR feedback

220b251


          Add more comments

1cff45a


          Implement aws.IsVirtualHostableS3Bucket function. (#16)

91a14a3


          Add condition methods to GetAttr

4d6d778


          Add functions for isVirtualHostableS3Bucket to Expr

65e8e9b


          Fix Parameters toBuilder

0f2b91c


          Remove the EndpointRulesetCustomization interface (#17)


          Add additional checks for invalid labels + ip addresses (#18)

5180a03


          Add a test case for empty host label

ff7a596


          First pass at cleanup

034bd7b

skmcgrail force-pushed the rules-engine branch from e7abe9c to 034bd7b Compare

September 30, 2022 00:20


          Fix substring

957379a

kstich requested changes

View reviewed changes

Contributor

kstich left a comment

Partially through, posting so edits can be made while review continues.

...-engine/src/main/java/software/amazon/smithy/rulesengine/validators/ValidationErrorType.java

+               * The type of validation error that has occurred.
+               */
+              @SmithyUnstableApi
+              public enum ValidationErrorType {

Contributor

kstich Sep 30, 2022

This can be an inner enum of ValidationError as Type if it sticks around, otherwise it seems like these are event ids.

...-engine/src/main/java/software/amazon/smithy/rulesengine/validators/ParametersValidator.java

+                                                  builder.type(ParameterType.BOOLEAN);
+                                                  break;
+                                              default:
+                                                  throw new IllegalArgumentException(java.lang.String.format(

Contributor

kstich Sep 30, 2022

Why a separate exception and not a validation event?

...ine/src/main/java/software/amazon/smithy/rulesengine/traits/EndpointTestsTraitValidator.java

+              public final class EndpointTestsTraitValidator extends AbstractValidator {
+                  @Override
+                  public List<ValidationEvent> validate(Model model) {
+                      return null;

Contributor

kstich Sep 30, 2022

Looks like this validation is missing or somewhere else meaning the class isn't needed anymore.

...c/main/java/software/amazon/smithy/rulesengine/language/syntax/parameters/ParameterType.java Show resolved Hide resolved

.../src/main/java/software/amazon/smithy/rulesengine/validators/StandaloneRulesetValidator.java

+                  private StandaloneRulesetValidator() {
+                  }
+                  public static Stream<ValidationError> validate(EndpointRuleSet ruleset, EndpointTestsTrait testSuite) {

Contributor

kstich Sep 30, 2022

How is this wired up to validate definitions automatically?

Why do the validators listed here not use the built in ValidationEvent system? I don't see anything in the validators that preclude using it. Multiple event IDs can be generated from the same validator by using the Builder instead of the ease-of-use functions.

smithy-rules-engine/src/main/java/software/amazon/smithy/rulesengine/language/Endpoint.java Show resolved Hide resolved

smithy-rules-engine/src/main/java/software/amazon/smithy/rulesengine/language/Endpoint.java Show resolved Hide resolved

smithy-rules-engine/src/main/java/software/amazon/smithy/rulesengine/language/Endpoint.java Show resolved Hide resolved

smithy-rules-engine/src/main/java/software/amazon/smithy/rulesengine/language/Endpoint.java Show resolved Hide resolved

...-rules-engine/src/main/java/software/amazon/smithy/rulesengine/language/EndpointRuleSet.java Show resolved Hide resolved

kstich reviewed

View reviewed changes

...-rules-engine/src/main/java/software/amazon/smithy/rulesengine/language/EndpointRuleSet.java Show resolved Hide resolved

...-rules-engine/src/main/java/software/amazon/smithy/rulesengine/language/EndpointRuleSet.java

+               * A set of EndpointRules. Endpoint Rules describe the endpoint resolution behavior for a service.
+               */
+              @SmithyUnstableApi
+              public final class EndpointRuleSet extends MandatorySourceLocation implements TypeCheck, ToNode {

Contributor

kstich Oct 4, 2022

Public methods are missing javadoc.

...-rules-engine/src/main/java/software/amazon/smithy/rulesengine/language/EndpointRuleSet.java

+                  }
+                  @Override
+                  public Type typeCheck(Scope<Type> scope) {

Contributor

kstich Oct 4, 2022

Could use some internal documentation.

...-rules-engine/src/main/java/software/amazon/smithy/rulesengine/language/EndpointRuleSet.java

+                      });
+                  }
+                  public void typecheck() {

Contributor

kstich Oct 4, 2022

This has different casing from typeCheck. It's also only used in a test and doesn't have the same return type. Is it needed?

...-rules-engine/src/main/java/software/amazon/smithy/rulesengine/language/EndpointRuleSet.java

+                  private Node rulesNode() {
+                      ArrayNode.Builder node = ArrayNode.builder();
+                      rules.forEach(node::withValue);
+                      return node.build();

Contributor

kstich Oct 4, 2022

This will return an empty array node [] even if there are no rules.

smithy-rules-engine/src/main/java/software/amazon/smithy/rulesengine/language/impl/AwsArn.java

+               * permissions and limitations under the License.
+               */
+              package software.amazon.smithy.rulesengine.language.impl;

Contributor

kstich Oct 4, 2022

This is the only thing in this package and it's AWS specific, should it move?

smithy-rules-engine/src/main/java/software/amazon/smithy/rulesengine/language/impl/AwsArn.java Show resolved Hide resolved

smithy-rules-engine/src/main/java/software/amazon/smithy/rulesengine/language/impl/AwsArn.java

+                      this.service = SmithyBuilder.requiredState("service", builder.service);
+                      this.region = SmithyBuilder.requiredState("region", builder.region);
+                      this.accountId = SmithyBuilder.requiredState("accountId", builder.accountId);
+                      this.resource = builder.resource.copy();

Contributor

kstich Oct 4, 2022

Is this required as well?

smithy-rules-engine/src/main/java/software/amazon/smithy/rulesengine/language/impl/AwsArn.java

+               * An AWS ARN.
+               */
+              @SmithyUnstableApi
+              public final class AwsArn implements ToSmithyBuilder<AwsArn> {

Contributor

kstich Oct 4, 2022

Needs builder and method level javadoc.

...rules-engine/src/main/java/software/amazon/smithy/rulesengine/language/stdlib/UriEncode.java

+               * permissions and limitations under the License.
+               */
+              package software.amazon.smithy.rulesengine.language.stdlib;

Contributor

kstich Oct 4, 2022

Maybe functions instead of stdlib?

Contributor

kstich commented Oct 4, 2022

Looks like Windows CI is failing as well, these are usually due to a path issue around / characters here.

alextwoods added 3 commits

October 7, 2022 15:39


          Replace newlines in invalidTestCase tests (possible windows fix)

d58ac06


          Compact whitespace in IntegrationTests#invalidTestCases

238a1b8


          Replace test paths in IntegrationTest with OS independent seperators

4f791a4

syall merged commit e15e3d9 into smithy-lang:main

This was referenced Mar 10, 2023

Service Specific endpoint - Review SEP awslabs/aws-sdk-swift#908

Closed

Endpoints 2.0 refactoring - Assessment awslabs/aws-sdk-swift#907

Closed

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

Labels

None yet