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
[CALCITE-6386] NPE when using ES adapter with model.json and no specified username, password or pathPrefix #3775
base: main
Are you sure you want to change the base?
Conversation
aeb19d5
to
e481252
Compare
…fied username, password or pathPrefix
e481252
to
efe9179
Compare
*/ | ||
@ResourceLock(value = "elasticsearch-scrolls", mode = ResourceAccessMode.READ) | ||
public class ElasticSearchAdapterFromModelFileTest { | ||
public static final EmbeddedElasticsearchPolicy NODE = EmbeddedElasticsearchPolicy.create(); |
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.
Can you include this in an existing test Java file. Too verbose.
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.
OK,I will add these tests to ElasticSearchAdapterTest
and update it.
.put("transport.tcp.port", 0) | ||
.put("http.port", 0) | ||
.put("transport.tcp.port", 9300) | ||
.put("http.port", 9200) |
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.
Why make this change? Back it out.
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.
I will discard this changes,thanks a lot.
"schemas": [ | ||
{ | ||
"type": "custom", | ||
"name": "elasticsearch", |
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.
No copyright header. No comments explaining its purpose. Looks benign but is broken.
Would be better to just remove it, if the test can be written without it.
try { | ||
CalciteAssert.that() | ||
.with(MODEL) | ||
.query(String.format("select * from \"%s\"", tableName)) |
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.
Test should fail if no exception is thrown.
.returnsCount(0); | ||
} catch (Exception e) { | ||
String message = e.getMessage(); | ||
Assertions.assertTrue(message.contains(String.format("Object '%s' not found", tableName))); |
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.
Use assertThat not assertTrue
List cacheKey = Arrays.asList(hosts, pathPrefix, username, password) | ||
.stream() | ||
.filter(Objects::nonNull) | ||
.collect(toImmutableList()); |
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.
(X, null, Y) will have the same cache key as (null, X, Y). This is a security flaw.
Suggest using an immutable map as cache key.
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.
Make sense. I will update it, thanks.
Quality Gate passedIssues Measures |
Details see: CALCITE-6386