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

Require the full path (without type) for field lookups #8872

Closed
clintongormley opened this issue Dec 10, 2014 · 9 comments
Closed

Require the full path (without type) for field lookups #8872

clintongormley opened this issue Dec 10, 2014 · 9 comments
Labels
>breaking :Search Foundations/Mapping Index mappings, including merging and defining field types Team:Search Foundations Meta label for the Search Foundations team in Elasticsearch v2.0.0-beta1

Comments

@clintongormley
Copy link
Contributor

Currently it is possible to refer to fields using:

  • a short name (eg first)
  • the full path (eg name.first)
  • the full path with the type prepended (eg employee.name.first) which automatically wraps a query clause with a filter on the _type field.

This method of field resolution is ambiguous.

Instead, we will require that the full path is used at all times, and that the type name cannot be prepended. If the user needs to filter on the _type field, then they should include an explicit filter.

Wildcarded field names will still be supported, eg name.* or *.first. Wildcards will apply to the whole path.

For bwc, we should add a static setting to allow the old method of field resolution in the 2.x series.

Relates to #8870

@clintongormley clintongormley added >breaking :Search Foundations/Mapping Index mappings, including merging and defining field types v2.0.0-beta1 labels Dec 10, 2014
@portante
Copy link

Will there be a grace period of some sort for compatibility? Or a way for folks to transition easily?

@clintongormley
Copy link
Contributor Author

@portante First, the full path is already supported, so you can start transitioning right away. Second, we should be able to provide a bwc setting to allow the old field resolution.

@rjernst
Copy link
Member

rjernst commented Dec 19, 2014

I'm more in favor of finding a path to remove completely in 2.0. I don't like the idea of a setting to "enable backcompat mode" since this setting would be from its inception throwaway. A setting wouldn't help people upgrading anyways, since it would be disabled by default. Instead, for 1.5 we could add a warning when a query uses simple names that they should change to the full path.

@clintongormley
Copy link
Contributor Author

@rjernst First, most people will go from 1.x to 2.y without passing through 1.5, so changes just in 1.5 won't be enough.

The second problem (which exists regardless of how or when we do this) is that today, people can query on non-existent fields, without any exception. It is perfectly acceptable to query all indices for name="foo" and to get results just from those indices that have a name field.

For this reason, I don't think we can warn about using the full path, because the path that the user has specified may well be the correct one. Really, the only approach to making users aware of this change is through blog posts and the breaking changes documentation.

Even with that, there is a good chance that many users will not read the docs and find themselves in a situation where none of their applications work. That's why I want the bwc setting, to give users a grace period to migrate.

If anybody has any better ideas I'm open to them.

@rjernst
Copy link
Member

rjernst commented Dec 19, 2014

It is perfectly acceptable to query all indices for name="foo" and to get results just from those indices that have a name field.

Yes but currently, if foo exists as a simple name in one of those indexes, then the code in question will be executed. I am suggesting a warning be added whenever that code is exercised.

For this reason, I don't think we can warn about using the full path, because the path that the user has specified may well be the correct one

The warning can be index specific, so if you query over multiple indexes, you get warnings for those indexes that the warning applies.

Even with that, there is a good chance that many users will not read the docs and find themselves in a situation where none of their applications work. That's why I want the bwc setting, to give users a grace period to migrate.

This bwc setting would enable them to use the old behavior throughout 2.y right? What happens when 3.z is released and they still haven't changed their client code? Or for those that complain about bugs in field resolution with this setting turned on (ie ambiguity which we are trying to fix)? In my experience, settings like this only prolong the problem.

Really, the only approach to making users aware of this change is through blog posts and the breaking changes documentation.

If that is the case, then why not make a blog post right now? And breaking changes documentation would exist with 2.0. I don't see how adding a setting for them to switch on is going to be that much better for them than saying "fix your queries".

@clintongormley
Copy link
Contributor Author

Yes but currently, if foo exists as a simple name in one of those indexes, then the code in question will be executed. I am suggesting a warning be added whenever that code is exercised.

Great idea. +1

This bwc setting would enable them to use the old behavior throughout 2.y right? What happens when 3.z is released and they still haven't changed their client code? Or for those that complain about bugs in field resolution with this setting turned on (ie ambiguity which we are trying to fix)? In my experience, settings like this only prolong the problem.

The point is to try to make it easy for users to upgrade, giving them sufficient time for migration. The more that we change between 1.x and 2.0, the harder it will be for them upgrade and the more likely that they will be stuck on an older version. There are things that we will have to break, but I'd prefer to keep these to a minimum.

While this strategy seems right to me, I'm not 100% convinced and would appreciate hearing from others, especially users.

@clintongormley
Copy link
Contributor Author

I've been thinking about this again. Users can start specifying full paths right now, so they already have a migration period - they could start changing their queries today.

The only tricky bit is: if a query uses a shortname by mistake, it will return no results, rather than an exception. While we could warn about missing fields, most people won't look at the logs.

In #6928 we added a strict query parsing mode for filtered aliases and percolator queries, which require fields to exist in the mapping before they can be used. This is disabled on normal queries. Perhaps we should allow users to specify strict field lookups in their queries, so that exceptions will be thrown if an unknown field is referenced?

@clintongormley
Copy link
Contributor Author

Related: #9521

rjernst added a commit to rjernst/elasticsearch that referenced this issue Feb 12, 2015
When multiple fields under object fields share the same name, accessing
by short name is ambiguous.  This removes support for short names,
always requiring the full name when used in queries.

closes elastic#8872
@dadoonet
Copy link
Member

Just a note here. I think this will break some tests in plugins such as lang plugins or rivers. Time to update plugins code I guess :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
>breaking :Search Foundations/Mapping Index mappings, including merging and defining field types Team:Search Foundations Meta label for the Search Foundations team in Elasticsearch v2.0.0-beta1
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants