DATAES-535 - Add mapping annotation @DynamicTemplates #238
Conversation
kukisak
commented
Jan 17, 2019
•
kukisak
commented
- You have read the Spring Data contribution guidelines.
- There is a ticket in the bug tracker for the project in our JIRA.
- You use the code formatters provided here and have them applied to your changes. Don’t submit any formatting related changes.
- You submit test cases (unit or integration tests) that back your changes.
- You added yourself as author in the headers of the classes you touched. Amend the date range in the Apache license header if needed. For new types, add the license header (copy from another file and set the current year only).
|
This pull request is addaptation of #132 to be compatible with spring-data-elasticsearch-3.1.x |
|
We can't accept your pull request without having a corresponding Jira issue. Please create one for this and adjust your commit message + pull request title according our Spring Data contribution guidelines. |
kukisak
changed the title
|
Hi @xhaggi , |
|
Please squash your commits into a single one and do a force-push to your branch. |
|
|
||
| /** | ||
| * Elasticsearch dynamic templates mapping. | ||
| * This annotation is handy if you prefer apply dynamic templates on fields with annotation e.g. {@link Field} with type = FieldType.Object etc. instead of static mapping on Document via {@link Mapping} annotation. |
There was a problem hiding this comment.
Please break down this comment.
| */ | ||
| private static void addDynamicTemplatesMapping(XContentBuilder builder, String jsonString) throws IOException { | ||
| JsonNode jsonNode = new ObjectMapper().readTree(jsonString).get("dynamic_templates"); | ||
| if (jsonNode != null && isNotBlank(jsonNode.toString()) && jsonNode.isArray()){ |
There was a problem hiding this comment.
IMO there is no need to check for isNotBlank(jsonNode.toString()) because jsonNode.isArray() should be enough.
| if (iterator.hasNext()){ | ||
| next = next.concat(","); | ||
| } | ||
| builder.rawValue(new BytesArray(next.getBytes()), XContentType.JSON); |
There was a problem hiding this comment.
Not sure if we really need to iterate over all node elements. It should be enough to write the jsonNode as raw value.
String json = objectMapper.writeValueAsString(jsonNode);
builder.field(FIELD_DYNAMIC_TEMPLATES);
builder.rawValue(new BytesArray(json.getBytes()), XContentType.JSON);
kukisak
force-pushed
the
dynamic-templates-for-spring-data-3.1.4
branch
from
7e3ae93 to
5738006
Compare
|
I applied all your requested changes. Please continue in review. |
| if (isNotBlank(mappingPath)) { | ||
| String jsonString = ElasticsearchTemplate.readFileFromClasspath(mappingPath); | ||
| if (isNotBlank(jsonString)) { | ||
| addDynamicTemplatesMapping(mapping, jsonString); |
There was a problem hiding this comment.
There's one little thing I'd like to change. I would move all this into addDynamicTemplatesMapping(), so we only have one call to this method and pass the XContentBuilder and class.
private static void addDynamicTemplatesMapping(XContentBuilder xContentBuilder, Class claszz) throws IOException
|
Ok, nice to see you've addressed all my concerns ;) Please squash all your commits into a single one and we are ready to shove it in. |
kukisak
force-pushed
the
dynamic-templates-for-spring-data-3.1.4
branch
from
68ad72d to
7c1dd4b
Compare
|
Ok, I squashed all commits to single one. So you can shove it in. |
Next time, please do not submit a pull request against a branch other than master. We do the backports when necessary. Right now it's fine, I'll adjust your changes to be based on the master. |
