Disallow type names to start with dots for new indices except for .percolator #12561
Conversation
jasontedor
added
>breaking
:Search/Mapping
|
This looks good to me, but I would like @clintongormley to confirm we want this change before merging. |
|
This looks good, however we should still allow for plugins to register reserved types? |
| if (Version.indexCreated(indexSettings).onOrAfter(Version.V_2_0_0_beta1) && typeContainsIllegalDot(mapper)) { | ||
| throw new IllegalArgumentException("mapping type name [" + mapper.type() + "] must not contain '.' in it"); | ||
| } | ||
| if (typeContainsIllegalDot(mapper)) { |
There was a problem hiding this comment.
maybe put this in an if else clause? For me this makes it clearer what is pre 2.0 and what is 2.0 behaviour.
|
Left one minor comment about the code. I do think we should mention this change in the |
If we think plugins should be able to reserve types, I'd rather not enforce anything in MapperService than make the list of reserved types pluggable. For the record, this would already be an issue today without @jasontedor 's pull request as we are logging a warning for types that contain a dot. |
|
I think we can replace "reserved type names" in the description of this PR with "percolator type". We don't have a mechanism to register reserved types. The fact that the I was expecting though any type with name that starts with "." would be hidden, like the |
jasontedor
changed the title
|
@martijnvg I agree with your suggestion on the code. I squashed a new commit for this that includes updated migration docs as well. |
|
LGTM. Let's go with this PR as is, and revisit reserving type names in plugins later. |
|
@clintongormley @jpountz mentioned getting confirmation from you before merging into master. |
|
Hmmm I can see that it could be a good thing to prevent type names from beginning with a dot, but what's the problem with them containing a dot? It used to be a problem when type names were used as a prefix to disambiguate fields, but that syntax is no longer allowed. Perhaps we should change the restriction to just prevent type names that begin with a dot? |
jasontedor
changed the title
|
@clintongormley Given that change, I agree. I've updated the PR. |
| import org.junit.Test; | ||
|
|
||
| public class MapperServiceTest extends ElasticsearchSingleNodeTest { | ||
| @Test(expected = IllegalArgumentException.class) |
There was a problem hiding this comment.
Instead of using expected exception, cant eh test wrap the call in a try/catch, and actually check the exception is what we think it should be? Something like:
try {
...prepareIndex...
} catch (IllegalArgumentException e) {
assertTrue(e.getMessage().contains("must not start with a '.'"));
}
Also I think it would be better to use an explicit create index call with mappings here, instead of relying on dynamic mappings?
There was a problem hiding this comment.
Agree on both counts, thanks!
|
@clintongormley Why would it be a good thing to prevent type names from beginning with a dot? IMHO with this PR aren't we disabling the ability for types to be hidden? |
|
I don't think we should allow either applications or plugins to reserve hidden types. I know we have been doing this with the percolator, but I don't think this is something we should replicate. In order to be useful, hidden types will likely need to store data, so they also need mappings. However in 2.0 we now check much more aggressively that all types have consistent mappings, so whenever a hidden type would add mappings to an index, it would restrict mappings that can be set on other regular types. If a plugin wants to reserve a namespace where it can store data, it should rather go with an index, like elasticsearch-core already does with the So +1 to merge and reject type names that start with a dot. |
I care to clarify that we don't have the ability for types to be hidden. Only the |
|
@javanna thanks for the clarification, makes sense. @jpountz I see where you are coming from, you'd rather encourage people to use an index instead of a new type. I'm thinking we could just restrict the usage of |
|
@alexksikes It reserves an entire class of names (those starting with dot) for future use. We can't reserve names in the future without potentially making breaking changes. This is why we want to do this on a major release when breaking changes are expected. |
…rcolator This commit will disallow type names to start with dots for version 2 and later indices except for .percolator. Closes #12560
Disallow type names to start with dots for new indices except for .percolator
This commit will disallow type names to start with dots except for .percolator for version 2 and later indices.
Closes #12560