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.

Sign up for GitHub

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

Disallow "enabled" attribute change for types in mapping update (#33566) #33933

Merged
merged 13 commits into from
Oct 1, 2018
Merged

Disallow "enabled" attribute change for types in mapping update (#33566) #33933

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
f09f1a1
Disallow "enabled" attribute change for types in mapping update (#33566)
cbismuth Sep 21, 2018
7e6cb90
Rename method in which mapping changes are resolved (#33566)
cbismuth Sep 21, 2018
ce31c85
Remove unnecessary type conditions (#33566)
cbismuth Sep 22, 2018
696b9a3
Remove unnecessary mapper exception constructor (#33566)
cbismuth Sep 22, 2018
c1db69e
Shorten "check 'enabled' field change" method name (#33566)
cbismuth Sep 22, 2018
e4458be
Revert unnecessary Git change (#33566)
cbismuth Sep 22, 2018
02b50dd
Private method can be static (#33566)
cbismuth Sep 24, 2018
df57bbb
Add test cases to object mapper merge operations (#33566)
cbismuth Sep 24, 2018
1a890a4
Delete unnecessary blank line (#33566)
cbismuth Sep 28, 2018
51feaae
Move Explicit allocations into method to ease readability (#33566)
cbismuth Sep 28, 2018
1489108
Use current version in index settings (#33566)
cbismuth Sep 28, 2018
760b6cc
Prefer mapper builders over constructors (#33566)
cbismuth Sep 28, 2018
ec6e768
Merge branch 'master' into 33566_disallow_type_enabled_mapping_update
cbismuth Oct 1, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 5 additions & 1 deletion server/src/main/java/org/elasticsearch/index/mapper/MapperException.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,11 +33,15 @@ public MapperException(String message) {
super(message);
}

public MapperException(String msg, Object... args) {
super(msg, args);
}
Copy link
Member

Choose a reason for hiding this comment

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

nit: not sure we need another constructor here, can you just insert the args into the message String in the call?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure.
Is LoggerMessageFormat#format preferred over String#format or maybe just string concatenation?


public MapperException(String message, Throwable cause) {
super(message, cause);
}

public MapperException(String message, Throwable cause, Object... args) {
super(message, cause, args);
}
}
}
21 changes: 21 additions & 0 deletions server/src/main/java/org/elasticsearch/index/mapper/ObjectMapper.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -458,6 +458,8 @@ protected void doMerge(final ObjectMapper mergeWith) {

for (Mapper mergeWithMapper : mergeWith) {
Mapper mergeIntoMapper = mappers.get(mergeWithMapper.simpleName());
throwExceptionIfEnabledAttributeIsUpdatedForType(mergeWith, mergeWithMapper, mergeIntoMapper);
Copy link
Member

Choose a reason for hiding this comment

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

nit: ImNotABigFanOfOverlyLongMethodNamesButWhatTheHeckIfYouReallyLikeIt ;-)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll shorten it, I'm not a big fan neither.
As I wasn't sure of the suggested design I've chosen (very) (very) explicit name to make myself understood.


Mapper merged;
if (mergeIntoMapper == null) {
// no mapping, simply add it
Expand All @@ -470,6 +472,25 @@ protected void doMerge(final ObjectMapper mergeWith) {
}
}

private void throwExceptionIfEnabledAttributeIsUpdatedForType(ObjectMapper mergeWith, Mapper mergeWithMapper, Mapper mergeIntoMapper) {
if (mergeIntoMapper instanceof ObjectMapper && mergeWithMapper instanceof ObjectMapper) {
final ObjectMapper mergeIntoObjectMapper = (ObjectMapper) mergeIntoMapper;
final ObjectMapper mergeWithObjectMapper = (ObjectMapper) mergeWithMapper;

if (mergeIntoObjectMapper.isEnabled() != mergeWithObjectMapper.isEnabled()) {
if (mergeIntoObjectMapper.nestedTypeFilter() instanceof TermQuery) {
Copy link
Member

Choose a reason for hiding this comment

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

I'm not quite sure I understand what the enable setting in mappings that are merged have to do with nested queries, especially with TermQuery. Maybe I'm missing sth., could you explain?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I dug into this with a debugger and the _type attribute happened to be wrapped into a TermQuery.

I've been a (tiny) bit paranoiac as I wanted to be sure to not make other things break.

I'll have another debugging session to see how things work when updating mapping with "_source": { "enabled": false }. I'll let you know.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Well, it looks like updating the _source attribute in a mapping is not allowed.

command

curl -XPUT "${ES_CLUSTER}/index1/_mapping/type1" -H "Content-Type: application/json" --data '{
  "properties": {
    "foo": {
      "source": {
        "enabled": false
      },
      "properties": {
        "bar": {
          "type": "text"
        }
      }
    }
  }
}'

output

{
  "error": {
    "root_cause": [
      {
        "type": "mapper_parsing_exception",
        "reason": "Mapping definition for [foo] has unsupported parameters:  [source : {enabled=false}]"
      }
    ],
    "type": "mapper_parsing_exception",
    "reason": "Mapping definition for [foo] has unsupported parameters:  [source : {enabled=false}]"
  },
  "status": 400
}

I'll now have a look if removing this conditional block doesn't conflict with other enabled-like metadata attributes.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No gradlew check -p server test failure after removing paranoiac conditional block 👍

final TermQuery termQuery = (TermQuery) mergeIntoObjectMapper.nestedTypeFilter();

Copy link
Member

Choose a reason for hiding this comment

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

nit: maybe delete this line

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, these two lines are related, no need to add a blank line, I'll remove it.

if (termQuery.getTerm() != null && "_type".equals(termQuery.getTerm().field())) {
final String path = mergeWith.fullPath() + "." + mergeWithObjectMapper.simpleName() + ".enabled";

throw new MapperException("Can't update attribute for type [{}] in index mapping", path);
}
}
}
}
}

@Override
public ObjectMapper updateFieldType(Map<String, MappedFieldType> fullNameToFieldType) {
List<Mapper> updatedMappers = null;
Expand Down