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

refactor: Call the correct pull/push query validator #5984

Merged
merged 2 commits into from
Aug 16, 2020

Conversation

spena
Copy link
Member

@spena spena commented Aug 11, 2020

Description

I found in the QueryAnalyzer code an invalid validator call to pull/push queries.

See the first QueryAnalyzer that calls another QueryAnalyzer constructor with push, then pull validators. But the constructor accepts pull, then push validators. Also the if condition in the analyze is calling a different validator.

public QueryAnalyzer(
      final MetaStore metaStore,
      final String outputTopicPrefix,
      final Set<SerdeOption> defaultSerdeOptions
  ) {
    this(
        new Analyzer(metaStore, outputTopicPrefix, defaultSerdeOptions),
        new PushQueryValidator(),
        new PullQueryValidator()
    );
  }

@VisibleForTesting
  QueryAnalyzer(
      final Analyzer analyzer,
      final QueryValidator pullQueryValidator,
      final QueryValidator pushQueryValidator
  ) {
    ...
  }

public Analysis analyze(...) {
     ...
     if (query.isPullQuery()) {
      pushQueryValidator.validate(analysis);
    } else {
      pullQueryValidator.validate(analysis);
    }
}

This PR fixes the constructor call and if condition to call the correct validator.

Testing done

Describe the testing strategy. Unit and integration tests are expected for any behavior changes.

Reviewer checklist

  • Ensure docs are updated if necessary. (eg. if a user visible feature is being added or changed).
  • Ensure relevant issues are linked (description should include text like "Fixes #")

@spena spena requested review from big-andy-coates and a team August 11, 2020 20:24
Copy link
Contributor

@big-andy-coates big-andy-coates left a comment

Choose a reason for hiding this comment

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

LGTM

@spena spena force-pushed the invalid_query_analyzer_validators branch from 725c227 to 5a81fc0 Compare August 13, 2020 20:39
@spena spena force-pushed the invalid_query_analyzer_validators branch from 5a81fc0 to 7aedb24 Compare August 16, 2020 11:16
@spena spena merged commit a89e525 into confluentinc:master Aug 16, 2020
@spena spena deleted the invalid_query_analyzer_validators branch August 16, 2020 13:01
sarwarbhuiyan pushed a commit to sarwarbhuiyan/ksql that referenced this pull request Sep 4, 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.

3 participants