fix: Parse expression with NBSP character #914

saig0 · 2024-08-28T11:16:04Z

Description

Adjust the parser's whitespace handling to ignore FEEL-specific whitespace characters, such as NBSP (i.e. Non-breaking space / \u00A0), as defined in the DMN specification.

The JavaWhitespace class worked very well because it also supported Java-like comments, as required by the DMN specification. Instead of adding additional whitespace rules for the individual parsers, extend the JavaWhitespace class by making a copy of it. 🙈 This should be less error-prone, easier to maintain, and more efficient. 👼

Related issues

closes #840

remcowesterhoud

Thanks @saig0 🚀 Please have a look a my comment, otherwise all good!

remcowesterhoud · 2024-08-29T07:41:18Z

src/test/scala/org/camunda/feel/impl/interpreter/InterpreterExpressionTest.scala

+  // FEEL-specific whitespace characters from the DMN specification
+  List(
+    '\u0009', '\u0020', '\u0085', '\u00A0', '\u1680', '\u180E', '\u2000', '\u2001', '\u2002',
+    '\u2003', '\u2004', '\u2005', '\u2006', '\u2007', '\u2008', '\u2009', '\u200A', '\u200B',
+    '\u2028', '\u2029', '\u202F', '\u205F', '\u3000', '\uFEFF', '\u000A', '\u000B', '\u000C',
+    '\u000D'
+  ).foreach { character =>
+    it should f"be parsed (\\u${character.toInt}%04X)" in {
+      evaluateExpression(s"${character}1") should returnResult(1)
+    }
+  }


🔧 Have you considered putting this in a parameterized test and running all the scenarios in this class for all whitespace characters?

Great idea. 💡

After migrating to a parameterized test, I found glitches with some whitespace characters. 👍

Add new test cases to verify that a NBSP and other space characters can be parsed.

Adjust the whitespace handling of the parser to ignore FEEL-specific whitespaces as defined in the DMN specification. The JavaWhitespace class worked very well because it supported also Java-like comments, as required by the DMN specification. Instead of adding additional whitespace rules for the individual parsers, extend the JavaWhitespace class by making a copy of it. This is less error-prone, easier to maintain, and more efficient.

Some whitespace characters were treated as part of the list iteration variable name. As a result, the parsing failed because the keyword "in" was not found. Fix the parsing issue by excluding whitespace characters as part of the variable name.

backport-action · 2024-08-29T12:03:37Z

Successfully created backport PR for 1.16:

[Backport 1.16] fix: Parse expression with NBSP character #915

backport-action · 2024-08-29T12:03:39Z

Successfully created backport PR for 1.17:

[Backport 1.17] fix: Parse expression with NBSP character #916

saig0 force-pushed the 840-whitespace-chars branch 2 times, most recently from 9bb774a to bacc0d8 Compare August 28, 2024 11:30

saig0 added backport 1.16 backport 1.17 labels Aug 28, 2024

saig0 requested a review from a team August 28, 2024 11:33

remcowesterhoud requested review from remcowesterhoud and removed request for a team August 29, 2024 07:33

remcowesterhoud approved these changes Aug 29, 2024

View reviewed changes

saig0 added 4 commits August 29, 2024 13:46

test: Verify space characters can be parsed

f8f3f78

Add new test cases to verify that a NBSP and other space characters can be parsed.

refactor: Migrate into a parameterized test

e965b88

fix: Ignore whitespace in list iteration

08ef5a4

Some whitespace characters were treated as part of the list iteration variable name. As a result, the parsing failed because the keyword "in" was not found. Fix the parsing issue by excluding whitespace characters as part of the variable name.

saig0 force-pushed the 840-whitespace-chars branch from 206ec2b to 08ef5a4 Compare August 29, 2024 11:57

saig0 added this pull request to the merge queue Aug 29, 2024

Merged via the queue into main with commit 97db122 Aug 29, 2024
5 checks passed

saig0 deleted the 840-whitespace-chars branch August 29, 2024 12:03

backport-action mentioned this pull request Aug 29, 2024

[Backport 1.16] fix: Parse expression with NBSP character #915

Merged

backport-action mentioned this pull request Aug 29, 2024

[Backport 1.17] fix: Parse expression with NBSP character #916

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: Parse expression with NBSP character #914

fix: Parse expression with NBSP character #914

saig0 commented Aug 28, 2024 •

edited

Loading

remcowesterhoud left a comment

remcowesterhoud Aug 29, 2024

saig0 Aug 29, 2024

saig0 Aug 29, 2024

backport-action commented Aug 29, 2024

backport-action commented Aug 29, 2024

fix: Parse expression with NBSP character #914

fix: Parse expression with NBSP character #914

Conversation

saig0 commented Aug 28, 2024 • edited Loading

Description

Related issues

remcowesterhoud left a comment

Choose a reason for hiding this comment

remcowesterhoud Aug 29, 2024

Choose a reason for hiding this comment

saig0 Aug 29, 2024

Choose a reason for hiding this comment

saig0 Aug 29, 2024

Choose a reason for hiding this comment

backport-action commented Aug 29, 2024

backport-action commented Aug 29, 2024

saig0 commented Aug 28, 2024 •

edited

Loading