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

Reading support for Elasticsearch #1999

Merged
merged 17 commits into from Jan 9, 2019

Conversation

Projects
None yet
8 participants
@meyerbaptiste
Copy link
Member

meyerbaptiste commented Jun 4, 2018

Q A
Bug fix? no
New feature? yes
BC breaks? no
Deprecations? yes
Tests pass? yes
Fixed tickets N/A
License MIT
Doc PR N/A
@soyuka
Copy link
Member

soyuka left a comment

quick comments, wanted to take a peek :D

@meyerbaptiste meyerbaptiste force-pushed the meyerbaptiste:elasticsearch branch 2 times, most recently from 22388ec to c68ab5e Jun 5, 2018

@Koc

This comment has been minimized.

Copy link

Koc commented Jun 9, 2018

What about using Elastica? It provides better api for building query http://elastica.io/elastica-vs-elasticsearch-php/

@dunglas

This comment has been minimized.

Copy link
Member

dunglas commented Jun 9, 2018

@Koc we benchmarked both solutions, but Elastica has no added value in our case.

$orders[] = $this->getOrder($resourceClass, $property, $direction);
}
} elseif (null !== $this->defaultDirection) {
foreach ($this->identifiersExtractor->getIdentifiersFromResourceClass($resourceClass) as $identifier) {

This comment has been minimized.

@epourail

epourail Jun 16, 2018

Contributor

You are assuming that an "identifier" property is always indexed. As, each document, in elasticsearch has the attribute _id, you can not be sure it is true.
The following use case fails : no "identifier" property indexed and get a collection with the default filter.

You should not apply the default condition and let elasticsearch retrieve the collection sorted by _id

However, as indicated in the ElasticSearch documentation, it is better to index the "identifier" field and duplicate the identifier content
(see https://www.elastic.co/guide/en/elasticsearch/reference/6.3/mapping-id-field.html#CO257-1)

This comment has been minimized.

@ro0NL

ro0NL Sep 1, 2018

The following use case fails : no "identifier" property indexed and get a collection with the default filter.

IMHO that's by design, you get the Composite identifiers not supported. exception right...

Currently it's consistent with the doctrine bridge: apply default order on ID fields.

@er1z

This comment has been minimized.

Copy link
Contributor

er1z commented Jul 19, 2018

Shouldn't this be published into separate bundle with API Platform as dependency? Core is getting more and more required packages whereas Elastic is not used everywhere.

@dunglas

This comment has been minimized.

Copy link
Member

dunglas commented Jul 19, 2018

@er1z, they are only dev packages, if you don't use it, you don't need the dep.

But, anyway, we plan to create a subtree split at some point, so we'll be able to cherry-pick only what we need.
Having a monolithic repository makes the maintenance easier (when our CI run, we test everything), and Elastic as well as MongoDB support are ones of the most frequent feature requests.

@ro0NL

This comment has been minimized.

Copy link

ro0NL commented Aug 31, 2018

@meyerbaptiste any chance the PR can be rebased? :) would like to try it out.

@soyuka soyuka force-pushed the meyerbaptiste:elasticsearch branch from 38ea274 to 76bd7e4 Aug 31, 2018

@soyuka

This comment has been minimized.

Copy link
Member

soyuka commented Aug 31, 2018

done @ro0NL

@ro0NL

This comment has been minimized.

Copy link

ro0NL commented Sep 1, 2018

I've created meyerbaptiste#4 to fix tests for now.

So far it works pretty well, nice :) would be nice if;

  • write ops are disabled by default
  • snake_case document fields are converted to camelCase object properties during serialization
  • a more convenient configuration IMO would be
elasticsearch:
  host: elasticsearch:9200
  index: api_documents
  mapping:
    App\Api\SomeResource: ~ # resource class as type, default index
   ...: { index: custom_index, type: custom_type }

@meyerbaptiste meyerbaptiste force-pushed the meyerbaptiste:elasticsearch branch 3 times, most recently from 2d8b178 to 0903c15 Nov 26, 2018

@meyerbaptiste

This comment has been minimized.

Copy link
Member Author

meyerbaptiste commented Nov 28, 2018

@ro0NL, I understand your point of view about the configuration. But we made the choice to only support a single index per document type (a resource in our case) because Elasticsearch is going to drop support for index types and will allow only a single type per index.

See https://www.elastic.co/guide/en/elasticsearch/reference/master/removal-of-types.html#_index_per_document_type.

  • write ops are disabled by default
  • snake_case document fields are converted to camelCase object properties during serialization

@meyerbaptiste meyerbaptiste force-pushed the meyerbaptiste:elasticsearch branch 2 times, most recently from 87e6024 to f3d9e13 Nov 30, 2018

@meyerbaptiste meyerbaptiste force-pushed the meyerbaptiste:elasticsearch branch 8 times, most recently from 875f843 to 0aa292b Dec 5, 2018

ro0NL and others added some commits Sep 1, 2018

@meyerbaptiste meyerbaptiste force-pushed the meyerbaptiste:elasticsearch branch from d0fc0f9 to 72f8e01 Jan 7, 2019

meyerbaptiste added some commits Dec 21, 2018

@meyerbaptiste meyerbaptiste force-pushed the meyerbaptiste:elasticsearch branch from 72f8e01 to 4d562dd Jan 7, 2019

@meyerbaptiste meyerbaptiste force-pushed the meyerbaptiste:elasticsearch branch 3 times, most recently from f3cf6a8 to 7a2b055 Jan 8, 2019

@er1z

This comment has been minimized.

Copy link
Contributor

er1z commented Jan 9, 2019

BTW, what version will be this issue merged to? Any changes for 2.x or does it break the APIs according to the semver?

@meyerbaptiste

This comment has been minimized.

Copy link
Member Author

meyerbaptiste commented Jan 9, 2019

@er1z this feature is planned for the next minor release (2.4).

@er1z

This comment has been minimized.

Copy link
Contributor

er1z commented Jan 9, 2019

@meyerbaptiste — can't wait for; feel free to ask if it needs any help I can contribute to.

@meyerbaptiste meyerbaptiste force-pushed the meyerbaptiste:elasticsearch branch from 7a2b055 to 53708b0 Jan 9, 2019

@meyerbaptiste

This comment has been minimized.

Copy link
Member Author

meyerbaptiste commented Jan 9, 2019

@er1z thanks, this PR is now ready but we will definitely need help to test the 2.4.0 beta 1 ☺️.

@meyerbaptiste meyerbaptiste force-pushed the meyerbaptiste:elasticsearch branch from 53708b0 to dfd0f36 Jan 9, 2019

@meyerbaptiste meyerbaptiste merged commit 89c0f30 into api-platform:master Jan 9, 2019

10 of 11 checks passed

SymfonyInsight Code quality below expectations.
Details
Scrutinizer Analysis: 9 new issues, 102 updated code elements – Tests: passed
Details
ci/circleci: behat-coverage Your tests passed on CircleCI!
Details
ci/circleci: merge-and-upload-coverage Your tests passed on CircleCI!
Details
ci/circleci: php-cs-fixer Your tests passed on CircleCI!
Details
ci/circleci: phpstan Your tests passed on CircleCI!
Details
ci/circleci: phpunit-coverage Your tests passed on CircleCI!
Details
codecov/patch 97.15% of diff hit (target 96.08%)
Details
codecov/project 96.13% (+0.04%) compared to 6d13f30
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@meyerbaptiste meyerbaptiste deleted the meyerbaptiste:elasticsearch branch Jan 9, 2019

@er1z
Copy link
Contributor

er1z left a comment

@meyerbaptiste — will take a deeper look in couple of days (Pagination, because I need this sooner than ES). Thanks.

throw new InvalidArgumentException('Page should not be greater than 1 if limit is equal to 0');
}
return [$page, $this->getOffset($resourceClass, $operationName), $limit];

This comment has been minimized.

@er1z

er1z Jan 10, 2019

Contributor

Why not ValueObject? It smells golang-way.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment