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

WIP: Do not force include_type_name #1654

Closed
wants to merge 1 commit into from

Conversation

thePanz
Copy link
Collaborator

@thePanz thePanz commented Aug 22, 2019

Do not arbitrary alter the requestEndpoint for index creation or when pushing mappings, the include_type_name should be configured on the endpoint, not added by the client

@ruflin
Copy link
Owner

ruflin commented Aug 27, 2019

We introduced this for compatibility with 7.0: #1563 I worry we might break something with this change. @p365labs Would be great to get your feedback on this one.

As we didn't ship 7.0 yet, we can still do breaking changes.

@thePanz thePanz force-pushed the es-7-do-not-force-include-type-name branch 2 times, most recently from 47974b9 to 0687884 Compare August 28, 2019 15:32
@thePanz thePanz force-pushed the es-7-do-not-force-include-type-name branch from 0687884 to 04aa560 Compare August 29, 2019 06:38
@ruflin
Copy link
Owner

ruflin commented Aug 29, 2019

@thePanz The include_type_name param will still exist in 7.0. You mention it should be configured by the endpoint, can you elaborate on how this would look like after this PR?

@thePanz
Copy link
Collaborator Author

thePanz commented Aug 29, 2019

@ruflin I expect these two different ways to create an index:

The one with type name included;

$create = new Create();
$create->setBody(['mappings' => ['type' => ['properties' => ['field1' => ['type' => 'text']]]]]);
$create->setParams(['include_type_name' => true]);
$index->requestEndpoint($create);

And without deprecation:

$create = new Create();
$create->setBody(['mappings' => ['properties' => ['field1' => ['type' => 'text']]]]);
$index->requestEndpoint($create);

WDYT?

Looks like the the current index->create() shortcut does not allow to pass any create parameters (thus the include_type_name).

@thePanz thePanz changed the title Do not force include_type_name WIP: Do not force include_type_name Aug 29, 2019
@thePanz
Copy link
Collaborator Author

thePanz commented Aug 29, 2019

Tests are failing, as this would require more work: we currently push the mappings with the type (we even require the type to be set in Mappings::toArray(), where we throw an exception if the type is not set).

@ruflin
Copy link
Owner

ruflin commented Sep 2, 2019

I like what you proposed above. It reduces the magic which is always good.

That we require the type in Mapping sounds now more like a bug. Any chance you could tackle this or is this a rabbit hole?

@thePanz
Copy link
Collaborator Author

thePanz commented Sep 18, 2019

@ruflin I will try to spend some time into this, no guarantees tho :)

@thePanz thePanz mentioned this pull request Oct 3, 2019
12 tasks
@thePanz
Copy link
Collaborator Author

thePanz commented Oct 30, 2019

Closing, as those changes are done in #1666

@thePanz thePanz closed this Oct 30, 2019
ruflin pushed a commit that referenced this pull request Oct 31, 2019
- [x] Remove mapping's  include_type_name (from #1654 )
- [x] Deprecate usage of Type object
- [x] copy `setMapping()` from `Type` to `Index`
- [x] copy `getDocument()` from `Type` to `Index`
- [x] copy `deleteById() from `Type` to `Index`
- [x] copy more methods from `Type` to `Index`
- [x] Cleanup `Client` methods requiring `Type` #1668
- [x] Remove `Type` class (<== needs clarification)
- [x] Cleanup `Search->addType()` #1669
- [x] Cleanup `Search->addTypes()` #1669
- [x] Cleanup `Bulk` class from `Type` #1670
- [x] Fix tests
@thePanz thePanz deleted the es-7-do-not-force-include-type-name branch November 20, 2019 14:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants