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

W-11222217: generate error message when attempting to migrate mel emp… #685

Merged
merged 5 commits into from
Aug 5, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,7 @@
<!-- * https://blogs.mulesoft.com/dev/mule-dev/why-dataweave-main-expression-language-mule-4-->
<!--Migration WARN: The MEL expression contains a method invocation that could not be migrated to a Dataweave expression.-->
<!-- For more information refer to:-->
<!-- * https://docs.mulesoft.com/mule-runtime/4.3/dataweave-cookbook-java-methods-->
<!-- * https://docs.mulesoft.com/dataweave/2.4/dataweave-cookbook-java-methods-->
<!-- * https://docs.mulesoft.com/mule-runtime/4.3/migration-mel-->
</set-payload>
</sub-flow>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -77,7 +77,7 @@
<!-- * https://blogs.mulesoft.com/dev/mule-dev/why-dataweave-main-expression-language-mule-4-->
<!--Migration WARN: The MEL expression contains a method invocation that could not be migrated to a Dataweave expression.-->
<!-- For more information refer to:-->
<!-- * https://docs.mulesoft.com/mule-runtime/4.3/dataweave-cookbook-java-methods-->
<!-- * https://docs.mulesoft.com/dataweave/2.4/dataweave-cookbook-java-methods-->
<!-- * https://docs.mulesoft.com/mule-runtime/4.3/migration-mel-->
</set-payload>
</sub-flow>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@

<!--Migration INFO: Email transformers are no longer needed. The payload already has the type information.-->
<!-- For more information refer to:-->
<!-- * https://docs.mulesoft.com/mule-runtime/4.3/intro-transformations#data-types-and-object-to-string-byte-inputstream-transformers-->
<!-- * https://docs.mulesoft.com/mule-runtime/4.3/intro-transformations#data-types-and-object-to-stringbyteinputstream-transformers-->
<!--<custom-transformer xmlns="http://www.mulesoft.org/schema/mule/core" name="rfc822-mime" class="org.mule.transport.email.transformers.Rfc822ByteArraytoMimeMessage" />-->
<email:pop3-config name="Pop3Config">
<email:pop3-connection host="localhost" port="${pop3Port}" user="bob" password="secret" />
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -78,4 +78,4 @@
"documentationLinks": []
}
]
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@
<!-- * https://blogs.mulesoft.com/dev/mule-dev/why-dataweave-main-expression-language-mule-4-->
<!--Migration WARN: The MEL expression contains a method invocation that could not be migrated to a Dataweave expression.-->
<!-- For more information refer to:-->
<!-- * https://docs.mulesoft.com/mule-runtime/4.3/dataweave-cookbook-java-methods-->
<!-- * https://docs.mulesoft.com/dataweave/2.4/dataweave-cookbook-java-methods-->
<!-- * https://docs.mulesoft.com/mule-runtime/4.3/migration-mel-->
</mule:set-payload>
{{/isWsdlEndpoint}}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -260,4 +260,4 @@
"documentationLinks": []
}
]
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -5,8 +5,8 @@
<!--Migration WARN: Proxy templates have changed in Mule 4. You can review them in Exchange to compare them to this migrated proxy.-->
<!-- For more information refer to:-->
<!-- * https://docs.mulesoft.com/api-manager/2.x/api-proxy-landing-page-->
<!--<custom-processor xmlns="http://www.mulesoft.org/schema/mule/core" class="com.mulesoft.gateway.extension.ProxyRequestHeadersProcessor">
<!--<custom-processor xmlns="http://www.mulesoft.org/schema/mule/core" class="com.mulesoft.gateway.extension.ProxyRequestHeadersProcessor">

</custom-processor>-->
</proxy:config>

Expand All @@ -20,8 +20,8 @@
<!--Migration WARN: APIs in Mule 4 have an identifier, the equivalent should be: ':'.-->
<!-- For more information refer to:-->
<!-- * https://docs.mulesoft.com/api-manager/2.x/configure-autodiscovery-4-task-->
<!--<api-platform-gw:api xmlns:api-platform-gw="http://www.mulesoft.org/schema/mule/api-platform-gw" apiName="![p['api.name']]" version="![p['api.version']]" flowRef="proxy">
<!--<api-platform-gw:api xmlns:api-platform-gw="http://www.mulesoft.org/schema/mule/api-platform-gw" apiName="![p['api.name']]" version="![p['api.version']]" flowRef="proxy">

</api-platform-gw:api>-->
</api-gateway:autodiscovery>

Expand Down Expand Up @@ -75,7 +75,7 @@
<!--Migration WARN: 'parseResponse' is not needed in Mule 4 because DataWeave 2.0 now handles those MIME types.-->
<!-- For more information refer to:-->
<!-- * https://docs.mulesoft.com/mule-runtime/4.3/migration-connectors-http#http-mime-types-->
<!-- * https://docs.mulesoft.com/mule-runtime/4.3/dataweave-formats#format_form_data-->
<!-- * https://docs.mulesoft.com/dataweave/2.4/dataweave-formats-->
<http:headers>#[migration::HttpRequester::httpRequesterHeaders(vars)]</http:headers>
<!--Migration WARN: Build the 'query-params' map with a single DataWeave expression.-->
<!-- For more information refer to:-->
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -376,4 +376,4 @@
"documentationLinks": []
}
]
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -5,8 +5,8 @@
<!--Migration WARN: Proxy templates have changed in Mule 4. You can review them in Exchange to compare them to this migrated proxy.-->
<!-- For more information refer to:-->
<!-- * https://docs.mulesoft.com/api-manager/2.x/api-proxy-landing-page-->
<!--<custom-processor xmlns="http://www.mulesoft.org/schema/mule/core" class="com.mulesoft.gateway.extension.ProxyRequestHeadersProcessor">
<!--<custom-processor xmlns="http://www.mulesoft.org/schema/mule/core" class="com.mulesoft.gateway.extension.ProxyRequestHeadersProcessor">

</custom-processor>-->
</proxy:config>

Expand Down Expand Up @@ -55,7 +55,7 @@
<!--Migration INFO: A 'parseRequest' is not needed in Mule 4 because the 'InputStream' of the multipart payload is provided as it is read.-->
<!-- For more information refer to:-->
<!-- * https://docs.mulesoft.com/mule-runtime/4.3/migration-connectors-http#http-mime-types-->
<!-- * https://docs.mulesoft.com/mule-runtime/4.3/dataweave-formats#format_form_data-->
<!-- * https://docs.mulesoft.com/dataweave/2.4/dataweave-formats-->
<http:error-response statusCode="#[vars.statusCode default migration::HttpListener::httpListenerResponseErrorStatusCode(vars)]">
<!--Migration WARN: Avoid using an outbound property to determine the status code.-->
<!-- For more information refer to:-->
Expand Down Expand Up @@ -90,7 +90,7 @@
<!--Migration WARN: 'parseResponse' is not needed in Mule 4 because DataWeave 2.0 now handles those MIME types.-->
<!-- For more information refer to:-->
<!-- * https://docs.mulesoft.com/mule-runtime/4.3/migration-connectors-http#http-mime-types-->
<!-- * https://docs.mulesoft.com/mule-runtime/4.3/dataweave-formats#format_form_data-->
<!-- * https://docs.mulesoft.com/dataweave/2.4/dataweave-formats-->
<http:headers>#[migration::HttpRequester::httpRequesterHeaders(vars)]</http:headers>
<!--Migration WARN: Build the 'query-params' map with a single DataWeave expression.-->
<!-- For more information refer to:-->
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -82,7 +82,7 @@
<!--Migration WARN: 'parseResponse' is not needed in Mule 4 because DataWeave 2.0 now handles those MIME types.-->
<!-- For more information refer to:-->
<!-- * https://docs.mulesoft.com/mule-runtime/4.3/migration-connectors-http#http-mime-types-->
<!-- * https://docs.mulesoft.com/mule-runtime/4.3/dataweave-formats#format_form_data-->
<!-- * https://docs.mulesoft.com/dataweave/2.4/dataweave-formats-->
<http:headers>#[migration::HttpRequester::httpRequesterHeaders(vars)]</http:headers>
<!--Migration WARN: Build the 'query-params' map with a single DataWeave expression.-->
<!-- For more information refer to:-->
Expand Down Expand Up @@ -148,7 +148,7 @@
<!-- * https://blogs.mulesoft.com/dev/mule-dev/why-dataweave-main-expression-language-mule-4-->
<!--Migration WARN: The MEL expression contains a method invocation that could not be migrated to a Dataweave expression.-->
<!-- For more information refer to:-->
<!-- * https://docs.mulesoft.com/mule-runtime/4.3/dataweave-cookbook-java-methods-->
<!-- * https://docs.mulesoft.com/dataweave/2.4/dataweave-cookbook-java-methods-->
<!-- * https://docs.mulesoft.com/mule-runtime/4.3/migration-mel-->
</set-payload>
</on-error-continue>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@ class MelGrammar(val input: ParserInput) extends Parser with StringBuilding {
private val createIfNode = (condition : MelExpressionNode, ifExpr : MelExpressionNode, elseExpr : MelExpressionNode) => IfNode(ifExpr, condition, elseExpr)
private val createPropertyNode = (identifiers: Seq[IdentifierNode]) => PropertyNode(identifiers)
private val createContainsNode = (left: MelExpressionNode, right: MelExpressionNode) => ContainsNode(left, right)
private val createEmptyLiteralNode = () => EmptyLiteralNode()


private val whiteSpaceChar = CharPredicate(" \f\t")
Expand Down Expand Up @@ -249,8 +250,12 @@ class MelGrammar(val input: ParserInput) extends Parser with StringBuilding {
booleanNode | numberNode | values
}

def emptyLiteral: Rule1[EmptyLiteralNode] = rule {
ws ~ str("empty") ~> createEmptyLiteralNode
}

def values = rule {
(property | constructor | methodInvocation | stringNodePreserveEscaping | stringNodeSimple | list | map | varReference | enclosedExpression) ~ zeroOrMore(selector)
(property | constructor | methodInvocation | stringNodePreserveEscaping | stringNodeSimple | list | map | emptyLiteral | varReference | enclosedExpression) ~ zeroOrMore(selector)
}

def constructor: Rule1[ConstructorNode] = rule {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -11,3 +11,5 @@ case class DefaultMigrationMetadata(override val children: Seq[MigrationMetadata
case class JavaModuleRequired() extends MigrationMetadata

case class NonMigratable(reason: String) extends MigrationMetadata

case class MigratableWithWarning(warning: String) extends MigrationMetadata
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ import org.mule.weave.v1.parser.Parser
import org.mule.weave.v2.{V1OperatorManager, V2LangMigrant}
import org.mule.weave.v2.grammar._
import org.mule.weave.v2.parser.ast.logical.{AndNode, OrNode}
import org.mule.weave.v2.parser.annotation.{EnclosedMarkAnnotation, InfixNotationFunctionCallAnnotation, QuotedStringAnnotation}
import org.mule.weave.v2.parser.annotation.{BooleanNotTypeAnnotation, EnclosedMarkAnnotation, InfixNotationFunctionCallAnnotation, NotType, QuotedStringAnnotation}
import org.mule.weave.v2.parser.ast.functions.FunctionCallParametersNode
import org.mule.weave.v2.parser.ast.header.HeaderNode
import org.mule.weave.v2.parser.ast.header.directives.{VersionDirective, VersionMajor, VersionMinor}
Expand All @@ -26,7 +26,7 @@ object Migrator {
val DEFAULT_HEADER = HeaderNode(Seq(VersionDirective(VersionMajor("2"), VersionMinor("0"))))
val CLASS_PROPERTY_NAME = "class"

def bindingContextVariable: List[String] = List("message", "exception", "payload", "flowVars", "sessionVars", "recordVars", "null");
def bindingContextVariable: List[String] = List("message", "exception", "payload", "flowVars", "sessionVars", "recordVars", "null", "empty");
var counter = 0


Expand All @@ -47,9 +47,14 @@ object Migrator {
case mel.MethodInvocationNode(canonicalName, arguments) => toDataweaveMethodInvocation(canonicalName, arguments)
case mel.PropertyNode(name) => toDataweaveProperty(name.map(_.literal).mkString("."))
case mel.ContainsNode(left, right) => toContainsInvocation(left, right)
case mel.EmptyLiteralNode() => toEmptyLiteral()
}
}

def toEmptyLiteral() = {
new MigrationResult(toDataweaveNameIdentifierNode("empty").dwAstNode, DefaultMigrationMetadata(Seq(MigratableWithWarning("expressions.emptyLiteral"))))
}

def toContainsInvocation(left: MelExpressionNode, right: MelExpressionNode): MigrationResult = {
val lRes = toDataweaveAst(left)
val rRes = toDataweaveAst(right)
Expand Down Expand Up @@ -191,8 +196,8 @@ object Migrator {
case mel.OperatorType.dot => toDataweaveBinaryOpNode(ValueSelectorOpId, left, right)
case mel.OperatorType.subscript => toDataweaveBinaryOpNode(DynamicSelectorOpId, left, right)
case mel.OperatorType.plus => toDataweaveBinaryOpNode(AdditionOpId, left, right)
case mel.OperatorType.equals => toDataweaveBinaryOpNode(EqOpId, left, right)
case mel.OperatorType.notEquals => toDataweaveBinaryOpNode(NotEqOpId, left, right)
case mel.OperatorType.equals => toDataweaveEqualityNode(EqOpId, left, right)
case mel.OperatorType.notEquals => toDataweaveEqualityNode(NotEqOpId, left, right)
case mel.OperatorType.lessThanOrEqual => toDataweaveBinaryOpNode(LessOrEqualThanOpId, left, right)
case mel.OperatorType.greaterThanOrEqual => toDataweaveBinaryOpNode(GreaterOrEqualThanOpId, left, right)
case mel.OperatorType.lessThan => toDataweaveBinaryOpNode(LessThanOpId, left, right)
Expand Down Expand Up @@ -226,6 +231,32 @@ object Migrator {
new MigrationResult(AndNode(lRes.dwAstNode, rRes.dwAstNode), DefaultMigrationMetadata(lRes.metadata.children ++ rRes.metadata.children))
}

private def toDataweaveEqualityNode(opId: BinaryOpIdentifier, left: MelExpressionNode, right: MelExpressionNode): MigrationResult = {
val lRes = toDataweaveAst(left)
val rRes = toDataweaveAst(right)

val nonEmptyRes = if (left.isInstanceOf[EmptyLiteralNode]) {
Some(rRes)
} else if (right.isInstanceOf[EmptyLiteralNode]) {
Some(lRes)
} else {
None
}

if (nonEmptyRes.isDefined) {
val param = FunctionCallParametersNode(Seq(nonEmptyRes.get.dwAstNode))
val variableReferenceNode = VariableReferenceNode(NameIdentifier("isEmpty"))
val node = if (opId == NotEqOpId) {
dw.operators.UnaryOpNode(NotOpId, dw.functions.FunctionCallNode(variableReferenceNode, param)).annotate(BooleanNotTypeAnnotation(NotType.Exclamation))
} else {
dw.functions.FunctionCallNode(variableReferenceNode, param)
}
new MigrationResult(node, DefaultMigrationMetadata(lRes.metadata.children ++ rRes.metadata.children))
} else {
new MigrationResult(dw.operators.BinaryOpNode(opId, lRes.dwAstNode, rRes.dwAstNode), DefaultMigrationMetadata(lRes.metadata.children ++ rRes.metadata.children))
}
}

private def toDataweaveBinaryOpNode(opId: BinaryOpIdentifier, left: MelExpressionNode, right: MelExpressionNode, metadata: MigrationMetadata = Empty()): MigrationResult = {
val lRes = toDataweaveAst(left)
val rRes = toDataweaveAst(right)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -57,6 +57,9 @@ case class ContainsNode(left: MelExpressionNode, right: MelExpressionNode) exten
case class DWFunctionNode(script: StringNode) extends MelExpressionNode {
}

case class EmptyLiteralNode() extends MelExpressionNode {
}

object OperatorType {
val plus = 0
val minus = 1
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
|EmptyLiteralNode
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
empty
Original file line number Diff line number Diff line change
Expand Up @@ -195,4 +195,16 @@ class MigratorTest extends FlatSpec with Matchers {
it should "migrate mel expression with isCausedBy function" in {
Migrator.migrate("exception.causedBy(org.mule.RuntimeException)").getGeneratedCode() shouldBe "%dw 2.0\n---\nJava::isCausedBy(error.cause, 'org.mule.RuntimeException', false)"
}

it should "migrate empty literal and generate warning" in {
val migrated = Migrator.migrate("payload == empty")
migrated.getGeneratedCode() shouldBe "%dw 2.0\n---\nisEmpty(payload)"
migrated.metadata.children.head shouldBe MigratableWithWarning("expressions.emptyLiteral")
}

it should "migrate empty literal inequality and generate warning" in {
val migrated = Migrator.migrate("payload != empty")
migrated.getGeneratedCode() shouldBe "%dw 2.0\n---\n!isEmpty(payload)"
migrated.metadata.children.head shouldBe MigratableWithWarning("expressions.emptyLiteral")
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we migrate to isEmpty(payload)

Copy link
Contributor Author

@andres-rad andres-rad Aug 3, 2022

Choose a reason for hiding this comment

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

I looked into it, but the empty comparison differs from isEmpty in a couple of cases:

  • 0 == empty and isEmpty(0)
  • false == empty and isEmpty(false)

Maybe the error message should be a bit more specific?

Copy link
Contributor

Choose a reason for hiding this comment

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

I see now we could migrate it to payload == 0 or payload == false or isEmpty(payload)

Copy link
Contributor

Choose a reason for hiding this comment

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

Or migrate it to just isEmpty(payload) with a warning saying that some use cases may not work. But I prefer to migrate it to something that just throw an error. I could imagine not many people using those weird semantics of the == empty

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I see now we could migrate it to payload == 0 or payload == false or isEmpty(payload)

I can add this, but there is one other case that I forgot to add, empty also returns true when compared against a string with only whitespaces. That is a case that we can't recognize because we would need to know if the argument is a String to trim it in the comparison.

I also don't imagine that a lot of people use this feature, but it feels wrong to migrate it with only a warning if we are potentially breaking the application. If you think it's not a problem I can migrate it to this case and add in the message that it breaks if the argument was a string with only whitespaces.

Copy link
Contributor

Choose a reason for hiding this comment

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

So it would be payload == 0 or payload == false or isEmpty(payload) or (payload is String and isBlank(payload)) The problem for me is that we have two options one is that we try to migrate with the most elegant but less compatible or we can try to migrate with the more compatible but less readable (elegant) that is this big expression. I'm ok with both things either migrate to isEmpty(payload) with a warning saying this may not work in all cases or migrate to ugly expression. @svacas what do you prefer?

Copy link
Contributor

Choose a reason for hiding this comment

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

I would go with the nice expression + warning, that should cover most of the cases and go along with the MMA philosophy of doing a best effort without getting too convoluted

Copy link
Contributor Author

@andres-rad andres-rad Aug 3, 2022

Choose a reason for hiding this comment

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

Nice expression being just isEmpty(payload) right?

Copy link
Contributor

Choose a reason for hiding this comment

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

yes

}
Original file line number Diff line number Diff line change
Expand Up @@ -110,6 +110,16 @@ public String translateSingleExpression(String unwrappedExpression, boolean data
return new DefaultMelCompatibilityResolver().resolve(unwrappedExpression, element, report, model, this, enricher);
}

if (result.metadata().children().exists(a -> a instanceof MigratableWithWarning)) {
List<MigratableWithWarning> metadata =
(List<MigratableWithWarning>) (List<?>) JavaConverters.seqAsJavaList(result.metadata().children())
.stream()
.filter(a -> a instanceof MigratableWithWarning)
.collect(toList());

metadata.forEach(a -> report.report(a.warning(), element, element));
}

if (migratedExpression.contains("message.inboundAttachments")) {
report.report("message.expressionsAttachments", element, element);
}
Expand Down
Loading