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

feat(stepfunctions): added new condition operators #9920

Merged
merged 3 commits into from
Sep 17, 2020
Merged

feat(stepfunctions): added new condition operators #9920

merged 3 commits into from
Sep 17, 2020

Conversation

michaelwiles
Copy link
Contributor

@michaelwiles michaelwiles commented Aug 23, 2020

Added the following Conditons:

  • Condition.isPresent - matches if a json path is present
  • Condition.isNotPresent - matches if a json path is not present
  • Condition.isString - matches if a json path contains a string
  • Condition.isNotString - matches if a json path is not a string
  • Condition.isNumeric - matches if a json path is numeric
  • Condition.isNotNumeric - matches if a json path is not numeric
  • Condition.isBoolean - matches if a json path is boolean
  • Condition.isNotBoolean - matches if a json path is not boolean
  • Condition.isTimestamp - matches if a json path is a timestamp
  • Condition.isNotTimestamp - matches if a json path is not a
    timestamp
  • Condition.isNotNull - matches if a json path is not null
  • Condition.isNull - matches if a json path is null
  • Condition.booleanEqualsJsonPath - matches if a boolean field equals
    a value in a given mapping path
  • Condition.stringEqualsJsonPath - matches if a string field equals a
    given mapping path
  • Condition.stringLessThanJsonPath - Matches if a string field sorts
    before a value at given mapping path
  • Condition.stringLessThanEqualsJsonPath - Matches if a string field
    sorts equal to or before a given mapping
  • Condition.stringGreaterThanJsonPath - Matches if a string field
    sorts after a value at a given mapping path
  • Condition.stringGreaterThanEqualsJsonPath - Matches if a string
    field sorts after or equal to value at a given mapping path
  • Condition.numberEqualsJsonPath - matches if a numeric field has the
    value in a given mapping path
  • Condition.numberLessThan - matches if a numeric field is less than
    the given value
  • Condition.numberLessThanJsonPath - matches if a numeric field is
    less than the value at the given mapping path
  • Condition.numberLessThanEqualsJsonPath - matches if a numeric field
    is less than or equal to the numeric value at given mapping path
  • Condition.numberGreaterThanJsonPath - matches if a numeric field is
    greater than the value at a given mapping path
  • Condition.numberGreaterThanEqualsJsonPath - matches if a numeric
    field is greater than or equal to the value at a given mapping path
  • Condition.timestampEqualsJsonPath - matches if a timestamp field is
    the same time as the timestamp at a given mapping path
  • Condition.timestampLessThanJsonPath - matches if a timestamp field
    is before the timestamp at a given mapping path
  • Condition.timestampLessThanEqualsJsonPath - matches if a timestamp
    field is before or equal to the timestamp at a given mapping path
  • Condition.timestampGreaterThanJsonPath - matches if a timestamp
    field is after the timestamp at a given mapping path
  • Condition.timestampGreaterThanEqualsJsonPath - matches if a
    timestamp field is after or equal to the timestamp at a given mapping
    path
  • Condition.stringMatches - matches a field with the ability to use as
    a wild card e.g: log-.txt or LATEST. No other characters other than ""
    have any special meaning (
    can be escaped: \*)

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license

@michaelwiles michaelwiles changed the title feat(aws-stepfunctions): added new condition operators closes #9821 feat(aws-stepfunctions): added new condition operators Aug 23, 2020
@shivlaks shivlaks self-requested a review August 31, 2020 20:59
Copy link
Contributor

@shivlaks shivlaks left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks for starting this @michaelwiles

  • The commit body should be updated with the list of condition operators we are adding support for.
  • can you update the README as well to include the new conditions
  • had a question about naming the methods that will support a JsonPath

Comment on lines 69 to 71
/**
* Tests if variable is not a timestamp
*/
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the terminology tests and matches are used. can we be consistent (I think I prefer Matches which already exists on a the comparison operators that are available today)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

happy with that

/**
* Matches if a boolean field has the given value
*/
public static booleanEquals(variable: string, value: boolean): Condition {
return new VariableComparison(variable, ComparisonOperator.BooleanEquals, value);
}

/**
* Matches if a boolean field has the given value
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
* Matches if a boolean field has the given value
* Matches if a boolean field has the given value path

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

will fix

/**
* Matches if a boolean field has the given value
*/
public static booleanEqualsPath(variable: string, value: string): Condition {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

wondering if Path is the right suffix for these methods?
I think we've been explicit about adding fromJsonPath when naming these methods. what do you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah - you have a point. In the Dynamo attribute value context json path is used.

I guess in this case I took inspiration from the cloudformation naming which doesn't use the json path.

I'm happy to stick with json path... don't have strong feelings against it

/**
* Matches if a numeric field is less than or equal to the given value
*/
public static numberLessThanEquals(variable: string, value: number): Condition {
return new VariableComparison(variable, ComparisonOperator.NumericLessThanEquals, value);
}

/**
* Matches if a numeric field is less than or equal to the numeric value at given maping path
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
* Matches if a numeric field is less than or equal to the numeric value at given maping path
* Matches if a numeric field is less than or equal to the numeric value at given mapping path

@@ -28,13 +28,85 @@ describe('Condition Variables', () => {
},
);
}),
test('Exercise a number of other conditions', () => {
test('Exercise string conditions', () => {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ideally we do not modify existing tests. it's preferred to only add new tests to ensure we don't introduce a regression. can we preserve this test as it was and test our conditions through new tests cases only?

@shivlaks
Copy link
Contributor

@yoodan93 @wong-a - can y'all have a look at this PR as well?

@mergify mergify bot dismissed shivlaks’s stale review September 13, 2020 08:28

Pull request has been modified.

@michaelwiles
Copy link
Contributor Author

@shivlaks changes implemented

@shivlaks shivlaks changed the title feat(aws-stepfunctions): added new condition operators feat(stepfunctions): added new condition operators Sep 15, 2020
@shivlaks shivlaks added the pr/do-not-merge This PR should not be merged at this time. label Sep 15, 2020
shivlaks
shivlaks previously approved these changes Sep 15, 2020
Copy link
Contributor

@shivlaks shivlaks left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@michaelwiles - changes look fine to me! can you update the commit body to include the implementation details / documentation, etc for this commit?

note: also added a do-not-merge label so that @yoodan93 and @wong-a have a chance to take a look at the changes as they might have additional suggestions.

Added the following Conditons:
* `Condition.isJsonPathPresent` - matches a json path is present
* `Condition.isJsonPathNotPresent` - matches a json path is not present
* `Condition.isJsonPathString` - matches a json path contains a string
* `Condition.isJsonPathNotString` - matches a json path is not a string
* `Condition.isJsonPathNumeric` - matches a json path is numeric
* `Condition.isJsonPathNotNumeric` - matches a json path is not numeric
* `Condition.isJsonPathBoolean` - matches a json path is boolean
* `Condition.isJsonPathNotBoolean` - matches a json path is not boolean
* `Condition.isJsonPathTimestamp` - matches a json path is a timestamp
* `Condition.isJsonPathNotTimestamp` - matches a json path is not a
timestamp
* `Condition.isJsonPathNotNull` - matches a json path is not null
* `Condition.isJsonPathNull` - matches a json path is null
* `Condition.booleanEqualsJsonPath` - matches a boolean field equals a
value in a given mapping path
* `Condition.stringEqualsJsonPath` - matches a string field equals a
given mapping path
* `Condition.stringLessThanJsonPath` - Matches a string field sorts
before a value at given mapping path
* `Condition.stringLessThanEqualsJsonPath` - Matches if a string field
sorts equal to or before a given mapping
* `Condition.stringGreaterThanJsonPath` - Matches if a string field
sorts after a value at a given mapping path
* `Condition.stringGreaterThanEqualsJsonPath` - Matches if a string
field sorts after or equal to value at a given mapping path
* `Condition.numberEqualsJsonPath` - Matches if a numeric field has the
value in a given mapping path
* `Condition.numberLessThan` - Matches if a numeric field is less than
the given value
* `Condition.numberLessThanJsonPath` - Matches if a numeric field is
less than the value at the given mapping path
* `Condition.numberLessThanEqualsJsonPath` - Matches if a numeric field
is less than or equal to the numeric value at given mapping path
* `Condition.numberGreaterThanJsonPath` - Matches if a numeric field is
greater than the value at a given mapping path
* `Condition.numberGreaterThanEqualsJsonPath` - Matches if a numeric
field is greater than or equal to the value at a given mapping path
* `Condition.timestampEqualsJsonPath` - Matches if a timestamp field is
the same time as the timestamp at a given mapping path
* `Condition.timestampLessThanJsonPath` - Matches if a timestamp field
is before the timestamp at a given mapping path
* `Condition.timestampLessThanEqualsJsonPath` - Matches if a timestamp
field is before or equal to the timestamp at a given mapping path
* `Condition.timestampGreaterThanJsonPath` - Matches if a timestamp
field is after the timestamp at a given mapping path
* `Condition.timestampGreaterThanEqualsJsonPath` - Matches if a
timestamp field is after or equal to the timestamp at a given mapping
path
* `Condition.stringMatches`
@mergify mergify bot dismissed shivlaks’s stale review September 15, 2020 07:29

Pull request has been modified.

@michaelwiles
Copy link
Contributor Author

commit body updated

Copy link

@yoodan93 yoodan93 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me!

@shivlaks
Copy link
Contributor

commit body updated

@michaelwiles by commit body i meant the description of this PR :)
sorry, i should have been clearer!!! - the PR description will be used as the commit body when merging

@@ -217,6 +217,54 @@ If your `Choice` doesn't have an `otherwise()` and none of the conditions match
the JSON state, a `NoChoiceMatched` error will be thrown. Wrap the state machine
in a `Parallel` state if you want to catch and recover from this.

#### Available Conditions:
see [step function comparison operators](https://docs.aws.amazon.com/step-functions/latest/dg/amazon-states-language-choice-state.html#amazon-states-language-choice-state-rules)
* `Condition.isJsonPathPresent` - matches a json path is present
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Am I missing something or do these isJsonPath<operator> conditions not exist? IsPresent is only accepts a JSONPath anyways.

Copy link
Contributor Author

@michaelwiles michaelwiles Sep 15, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not sure what you are asking?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I meant that isJsonPathPresent, isJsonPathNotPresent, etc. aren't defined in the Condition class

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very good point - thank you. The jsonpath here is superfluous and does not match the code - will address.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

updated accordingly - made README match the code.

* `Condition.isJsonPathNotTimestamp` - matches a json path is not a timestamp
* `Condition.isJsonPathNotNull` - matches a json path is not null
* `Condition.isJsonPathNull` - matches a json path is null
* `Condition.booleanEquals` - matches a boolean field has a given value
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: Can we consistent text as the comment on the actual conditions? The casing and structure also differs from lines below.

Suggested change
* `Condition.booleanEquals` - matches a boolean field has a given value
* `Condition.booleanEquals` - Matches if a boolean field has a given value

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

and fixed consistency of case and wording

Added the following Conditons:
* `Condition.isPresent` - matches if a json path is present
* `Condition.isNotPresent` - matches if a json path is not present
* `Condition.isString` - matches if a json path contains a string
* `Condition.isNotString` - matches if a json path is not a string
* `Condition.isNumeric` - matches if a json path is numeric
* `Condition.isNotNumeric` - matches if a json path is not numeric
* `Condition.isBoolean` - matches if a json path is boolean
* `Condition.isNotBoolean` - matches if a json path is not boolean
* `Condition.isTimestamp` - matches if a json path is a timestamp
* `Condition.isNotTimestamp` - matches if a json path is not a
timestamp
* `Condition.isNotNull` - matches if a json path is not null
* `Condition.isNull` - matches  if a json path is null
* `Condition.booleanEqualsJsonPath` - matches if a boolean field equals
a value in a given mapping path
* `Condition.stringEqualsJsonPath` - matches if a string field equals a
given mapping path
* `Condition.stringLessThanJsonPath` - Matches if a string field sorts
before a value at given mapping path
* `Condition.stringLessThanEqualsJsonPath` - Matches if a string field
sorts equal to or before a given mapping
* `Condition.stringGreaterThanJsonPath` - Matches if a string field
sorts after a value at a given mapping path
* `Condition.stringGreaterThanEqualsJsonPath` - Matches if a string
field sorts after or equal to value at a given mapping path
* `Condition.numberEqualsJsonPath` - matches if a numeric field has the
value in a given mapping path
* `Condition.numberLessThan` - matches if a numeric field is less than
the given value
* `Condition.numberLessThanJsonPath` - matches if a numeric field is
less than the value at the given mapping path
* `Condition.numberLessThanEqualsJsonPath` - matches if a numeric field
is less than or equal to the numeric value at given mapping path
* `Condition.numberGreaterThanJsonPath` - matches if a numeric field is
greater than the value at a given mapping path
* `Condition.numberGreaterThanEqualsJsonPath` - matches if a numeric
field is greater than or equal to the value at a given mapping path
* `Condition.timestampEqualsJsonPath` - matches if a timestamp field is
the same time as the timestamp at a given mapping path
* `Condition.timestampLessThanJsonPath` - matches if a timestamp field
is before the timestamp at a given mapping path
* `Condition.timestampLessThanEqualsJsonPath` - matches if a timestamp
field is before or equal to the timestamp at a given mapping path
* `Condition.timestampGreaterThanJsonPath` - matches if a timestamp
field is after the timestamp at a given mapping path
* `Condition.timestampGreaterThanEqualsJsonPath` - matches if a
timestamp field is after or equal to the timestamp at a given mapping
path
* `Condition.stringMatches` - matches a field with the ability to use as
a wild card e.g: log-.txt or LATEST. No other characters other than "*"
have any special meaning (* can be escaped: \\*)
@michaelwiles
Copy link
Contributor Author

@shivlaks please can you move this along? we want to use these new conditions...

@shivlaks shivlaks removed the pr/do-not-merge This PR should not be merged at this time. label Sep 17, 2020
@shivlaks
Copy link
Contributor

@michaelwiles thanks for the contribution! dropped the do-not-merge label. This feature will be included in the next release.

@michaelwiles
Copy link
Contributor Author

@shivlaks it looks like you still need to approve the pull request..?

@aws-cdk-automation
Copy link
Collaborator

AWS CodeBuild CI Report

  • CodeBuild project: AutoBuildProject6AEA49D1-qxepHUsryhcu
  • Commit ID: 137fd27
  • Result: SUCCEEDED
  • Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

@mergify
Copy link
Contributor

mergify bot commented Sep 17, 2020

Thank you for contributing! Your pull request will be updated from master and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

@mergify mergify bot merged commit b8490f2 into aws:master Sep 17, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants