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

Duplicates facet does not escape its column name, resulting in an invalid GREL expression #4233

Closed
wetneb opened this issue Oct 21, 2021 · 5 comments · Fixed by #4538
Closed
Labels
facets Behaviour or rendering of facets in a project Good First Issue Indicates issues suitable for newcomers to design or coding, providing a gentle introduction. Module: Frontend These issues involve working on HTML, CSS, and JavaScript code that affects the user interface. Type: Bug Issues related to software defects or unexpected behavior, which require resolution.
Milestone

Comments

@wetneb
Copy link
Sponsor Member

wetneb commented Oct 21, 2021

The code that generates the GREL expression for the Duplicates facet does not escape the name of the selected column.
If this name contains a single quote character, this results in an invalid GREL expression, hence the facet cannot be used as such.

To Reproduce

Steps to reproduce the behavior:

  1. First, import the following CSV file:
Jane's column,Jack's column
foo,bar
hey,ho
hey,bar
  1. Then, create a Duplicates facet on any of the column (using the column menu)

Current Results

The facet's GREL expression is invalid: facetCount(value, 'value', 'Jane's column') > 1.

Expected Behavior

The GREL expression should be valid, for instance facetCount(value, 'value', 'Jane\'s column') > 1 would work.

Versions

  • Operating System: irrelevant
  • Browser Version: irrelevant
  • JRE or JDK Version: irrelevant
  • OpenRefine: master

Additional context

Brought up on our mailing list: https://groups.google.com/g/openrefine/c/2ic5F0YGcbk

@wetneb wetneb added Type: Bug Issues related to software defects or unexpected behavior, which require resolution. Module: Frontend These issues involve working on HTML, CSS, and JavaScript code that affects the user interface. Good First Issue Indicates issues suitable for newcomers to design or coding, providing a gentle introduction. facets Behaviour or rendering of facets in a project labels Oct 21, 2021
@thadguidry
Copy link
Member

Hmm, but we have always allowed single or double quotes in the expressions, does that not help enough here?

@wetneb
Copy link
Sponsor Member Author

wetneb commented Oct 22, 2021

I do not know what you mean by "help enough" here :) what solution do you have in mind?

@thadguidry
Copy link
Member

I meant switching to double quotes versus only using single quotes.

If the value includes single quotes, then double quote wrapping should work. Escaping wouldn't be needed.
If the value includes double quotes, then single quote wrapping should suffice. Escaping wouldn't be needed.
if the value included double and single quotes, then single quote wrapping with escaping would be the only way to avoid the error.

Currently with 3.5 beta 1
image
image

@wetneb
Copy link
Sponsor Member Author

wetneb commented Oct 22, 2021

Yes, so you cannot avoid escaping entirely. I am not sure if it is worth adding extra logic to choose between quote types, it is probably easier to stick to one syntax and always escape for it.

@thadguidry
Copy link
Member

Agree. Just mentioning how things have worked since the beginning of our code.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
facets Behaviour or rendering of facets in a project Good First Issue Indicates issues suitable for newcomers to design or coding, providing a gentle introduction. Module: Frontend These issues involve working on HTML, CSS, and JavaScript code that affects the user interface. Type: Bug Issues related to software defects or unexpected behavior, which require resolution.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants