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

match_query: unit test and rewrite for TypeScript #208

Merged
merged 3 commits into from
Mar 18, 2024
Merged

Conversation

liamwhite
Copy link
Contributor

This moves all of the match_query clientside parsing library into TypeScript. There should be no substantial logic changes to the internal parse logic here, but three things were discovered during testing and fixed:

  1. Incorrect escaped quote processing in terms. This arose from the translation of the following Ruby code
    def normalize_term!(match, quoted)
      if quoted
        match.gsub!('\"', '"')
      else
        match.gsub!(/\\(.)/, '\1')
      end
    end
    as
    SearchTerm.prototype._normalizeTerm = function() {
      if (!this.wildcardable) {
        return this.term.replace('\"', '"');
      }
      return this.term.replace(/\\([^\*\?])/g, '$1');
    };
    which is a very subtle mistake, as the string literal escaping rules are different in JS.
  2. Inconsistency in relative date parsing. For absolute dates, the lte and gt operators use the "upper" date in the date range, and the lt and gte operators use the "lower" date. For relative dates, only the "lower" date was used. This is inconsistent with the behavior of the server-side parser, which uses the same behavior for both date types, so it was changed to match.
  3. Use of the deprecated/optional substr JS API. These correspond to the slice method used in the Ruby parser.
    # Truncate string and restart the token tests.
    @search_str = @search_str.slice(match.size,
                                    @search_str.size - match.size)
    Just a minor fix to change these to substring.

This has 100% test coverage in unit tests and has also been manually integration tested.

assets/js/query/term.ts Outdated Show resolved Hide resolved
assets/js/query/lex.ts Outdated Show resolved Hide resolved
assets/js/query/date.ts Outdated Show resolved Hide resolved
@liamwhite liamwhite merged commit 3590be1 into master Mar 18, 2024
4 checks passed
@liamwhite liamwhite deleted the ts-query branch March 18, 2024 12:20
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.

2 participants