[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.
|
Details see: CALCITE-6386