[CALCITE-6386] NPE when using ES adapter with model.json and no specified username, password or pathPrefix #3775
Conversation
guluo2016
force-pushed
the
es-apater-model-json
branch
2 times, most recently
from
aeb19d5 to
e481252
Compare
…fied username, password or pathPrefix
guluo2016
force-pushed
the
es-apater-model-json
branch
from
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.
Can you include this in an existing test Java file. Too verbose.
There was a problem hiding this comment.
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.
Why make this change? Back it out.
There was a problem hiding this comment.
I will discard this changes,thanks a lot.
| "schemas": [ | ||
| { | ||
| "type": "custom", | ||
| "name": "elasticsearch", |
There was a problem hiding this comment.
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.
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.
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.
(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.
Make sense. I will update it, thanks.
|
|
This pull request has been marked as stale due to 30 days of inactivity. It will be closed in 90 days if no further activity occurs. If you think that’s incorrect or this pull request requires a review, please simply write any comment. If closed, you can revive the PR at any time and @mention a reviewer or discuss it on the [email protected] list. Thank you for your contributions. |
Details see: CALCITE-6386