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

cfontes
Copy link
Contributor

@cfontes cfontes 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 SupportEmptyFieldsArrayInMappings branch 4 times, most recently from e615f8d to b43c2ce Compare August 26, 2014 04:15
@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) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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());
  }
}

@jpountz
Copy link
Contributor

jpountz 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
Copy link
Contributor

jpountz 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
Copy link
Contributor Author

cfontes 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
Copy link
Contributor Author

cfontes 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
Copy link
Contributor

jpountz commented Aug 27, 2014

Merged, thanks!

@cfontes cfontes deleted the SupportEmptyFieldsArrayInMappings branch August 29, 2014 04:46
areek pushed 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 added the :Core/Infra/REST API REST infrastructure and utilities label Jun 7, 2015
@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
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants