Skip to content
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

Added support for empty field arrays in mappings #7271

Closed

Conversation

Projects
None yet
3 participants
@cfontes
Copy link
Contributor

commented Aug 14, 2014

Fix for #6133, added the ability to send empty arrays as part of an index mapping json. For single and multi field properties objects

Sorry for the import changes, IntelliJ doesn't like eclipse imports and fights it like the devil.

If the test are in a non standard format please advise. I just find them easier to read like this.

@cfontes cfontes force-pushed the cfontes:SupportEmptyFieldsArrayInMappings branch 4 times, most recently to b43c2ce Aug 26, 2014

@jpountz jpountz self-assigned this Aug 26, 2014

@@ -248,7 +247,7 @@ public static void parseField(AbstractFieldMapper.Builder builder, String name,
public static void parseMultiField(AbstractFieldMapper.Builder builder, String name, Map<String, Object> node, Mapper.TypeParser.ParserContext parserContext, String propName, Object propNode) {
if (propName.equals("path")) {
builder.multiFieldPathType(parsePathType(name, propNode.toString()));
} else if (propName.equals("fields")) {
} else if (propName.equals("fields") && propNode instanceof Map) {

This comment has been minimized.

Copy link
@jpountz

jpountz Aug 26, 2014

Contributor

I think we should not just ignore when something else than a map is provided? Maybe we could do something like:

} else if (propName.equals("fields") {
  final Map<String, Object> multiFieldsPropNodes;
  if (propNode instance of List && ((List<?>) propNode.isEmpty()) {
    multiFieldsPropNodes = Collections.emptyMap();
  } else if (propNode instanceof Map) {
    multiFieldsPropNodes = (Map<String, Object>) propNode;
  } else {
    throw new MapperParsingException("Expected map for property [fields] on field [" + multiFieldName + "] or [" + type + "] but got a " + propNode.getClass());
  }
}
objBuilder.add(typeParser.parse(propName, propNode, parserContext));
String propName = entry.getKey();

if(entry.getValue() instanceof Map) {

This comment has been minimized.

Copy link
@jpountz

jpountz Aug 26, 2014

Contributor

I have a similar concern here for when entry.getValue is not a Map

@jpountz

This comment has been minimized.

Copy link
Contributor

commented Aug 26, 2014

@cfontes I left two comments on the PR but otherwise it looks good, especially the tests.

I would be nice to fix these imports. I'm quite surprised that you mention that you use Intellij since I thought we were using Intellij's defaults (we even have an eclipse configuration to make sure we are using Intellij's import style). Or maybe you have a non-default configuration of imports in Intellij?

Could you please also sign our contributor license agreement so that we can eventually merge this pull request?

Thanks!

@jpountz jpountz removed the review label Aug 26, 2014

@jpountz

This comment has been minimized.

Copy link
Contributor

commented Aug 26, 2014

Could you please also sign our contributor license agreement so that we can eventually merge this pull request?

I just learned that it is in, so it is allright for the CLA.

@cfontes

This comment has been minimized.

Copy link
Contributor Author

commented Aug 27, 2014

Thanks for the review.

I will look into fixing those and push it asap.

About the imports, you are right... it's my bad, one of my other projects have a very specific rule for imports and it got into my ES project by mistake. Will fix that too.

Cheers!

@cfontes

This comment has been minimized.

Copy link
Contributor Author

commented Aug 27, 2014

@jpountz

Added back the IntelliJ default import settings.
Added some code to validate the scenarios explained.
Added some tests to validate those.

Since there is nothing really to do in parseProperties when the parameter is an empty List it just skip the parsing and proceed without any exception since now it's a valid input that does nothing. If it's anything else (besides a map) it throws an exception. Please take a look to see if it's OK like that for you.

Thanks!

@jpountz

This comment has been minimized.

Copy link
Contributor

commented Aug 27, 2014

Merged, thanks!

@jpountz jpountz added v2.0.0 labels Aug 27, 2014

@cfontes cfontes deleted the cfontes:SupportEmptyFieldsArrayInMappings branch Aug 29, 2014

areek added a commit that referenced this pull request Sep 8, 2014

@clintongormley clintongormley changed the title Added support for empty field arrays in mappings REST API: Added support for empty field arrays in mappings Sep 8, 2014

@clintongormley clintongormley changed the title REST API: Added support for empty field arrays in mappings Added support for empty field arrays in mappings Jun 7, 2015

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.