-
Notifications
You must be signed in to change notification settings - Fork 24.3k
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
[Connector API] Change UpdateConnectorFiltering API to have better defaults #108612
[Connector API] Change UpdateConnectorFiltering API to have better defaults #108612
Conversation
Pinging @elastic/ent-search-eng (Team:Enterprise Search) |
c7845b0
to
39dbd9a
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! Great to see more folks contributing to ES! I left some nit comments about adding more tests, comments and minor refactoring
.findFirst(); | ||
|
||
List<FilteringRule> sortedRules = rules.stream() | ||
.filter(rule -> rule.equalsExceptForTimestampsAndOrder(getDefaultFilteringRule(null, 0)) == false) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe let's create another method isDefaultFilteringRule
, it might be confusing why we do this:
rule.equalsExceptForTimestampsAndOrder(getDefaultFilteringRule(null, 0))
...t-search/src/main/java/org/elasticsearch/xpack/application/connector/ConnectorFiltering.java
Show resolved
Hide resolved
@@ -196,9 +196,22 @@ private static FilteringValidation getRandomFilteringValidationError() { | |||
.build(); | |||
} | |||
|
|||
public static FilteringRule getDefaultFilteringRule(Instant currentTimestamp, Integer order) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is accessed from within non test code, so maybe let's move it to ConnetorFiltering.java
file?
Also we call it a lot with getDefaultFilteringRule(null, 0)
so we can use method overloading or add WithOrder
explicitly at the end of the method name:
public static FilteringRule getDefaultFilteringRuleWithOrder(Integer order) {
getDefaultFilteringRule(null, order)
}
.../src/test/java/org/elasticsearch/xpack/application/connector/ConnectorIndexServiceTests.java
Show resolved
Hide resolved
...lRestTest/resources/rest-api-spec/test/entsearch/connector/60_connector_update_filtering.yml
Show resolved
Hide resolved
Hi @efegurkan, I've created a changelog YAML for you. |
This changes validation behaviour for rules to have better defaults instead of throwing. The following cases changed to not throw: - When "rules" is an empty array, instead of throwing we attach a default rule - When "rules" have items but doesn't have default rule, we append a default rule. - Rules array will be ordered and default will be moved as the last
c33e50a
to
0dcdccb
Compare
Hi @efegurkan, I've created a changelog YAML for you. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGMT! Thank you for looking into this
…faults (elastic#108612) * Change UpdateConnectorFiltering API to have better defaults This changes validation behaviour for rules to have better defaults instead of throwing. The following cases changed to not throw: - When "rules" is an empty array, instead of throwing we attach a default rule - When "rules" have items but doesn't have default rule, we append a default rule. - Rules array will be ordered and default will be moved as the last * Update docs/changelog/108612.yaml
…faults (elastic#108612) * Change UpdateConnectorFiltering API to have better defaults This changes validation behaviour for rules to have better defaults instead of throwing. The following cases changed to not throw: - When "rules" is an empty array, instead of throwing we attach a default rule - When "rules" have items but doesn't have default rule, we append a default rule. - Rules array will be ordered and default will be moved as the last * Update docs/changelog/108612.yaml
This changes validation behaviour for rules to have better defaults instead of throwing. The following cases changed to not throw: