Add initial functional tests #1

dkarlovi · 2017-10-17T15:15:07Z

dkarlovi · 2017-10-17T15:15:53Z

@sstok is this something you had in mind?

sstok · 2017-10-18T07:27:48Z

.gitignore

@@ -1,3 +1,4 @@
+/.idea/


Please remove this, you can use the global .gitignore instead 👍
https://help.github.com/articles/ignoring-files/#create-a-global-gitignore

Done, thanks for the link.

sstok · 2017-10-18T11:22:43Z

Thank you so much for your work so far 👍

dkarlovi · 2017-10-18T11:47:35Z

@sstok can you elaborate a bit on the registerField()? How is that supposed to work exactly?

ie. how do I map from fieldset customer-name to index invoices, type invoices, property customer.full_name?

dkarlovi · 2017-10-18T13:07:37Z

tests/Functional/FunctionalElasticsearchTestCase.php

+    protected function configureConditionGenerator(QueryConditionGenerator $conditionGenerator)
+    {
+        // TODO: mapping field set alias to Elasticsearch property
+        return;


@sstok this is where I'm hoping you can shed some light. :)

dkarlovi · 2017-10-18T13:08:34Z

tests/Functional/FunctionalElasticsearchTestCase.php

+        $conditionGenerator = new QueryConditionGenerator($condition);
+        $this->configureConditionGenerator($conditionGenerator);
+
+        // TODO: where do I get these from?


@sstok also here, don't quite understand how you imagined for it to work.

sstok · 2017-10-18T13:24:59Z

The registerField method works similar to the Doctrine DBAL/ORM implementation 😃
You should be able to map a search field to one or more fields, but ElasticSearch actually is much more advanced in this topic as (IIRC) you can create an index with combining fields.

It's a while since I worked on the ElasticSearch implementation 😅

Backed by Elasticsearch 5.6

dkarlovi · 2017-10-18T13:36:25Z

@sstok sure, we can work on advanced mappings later, for now let's try and get anything working. :)

For example, how did you imagine we map id: 1 to /invoices/invoices, property _id with the registerField()?

dkarlovi · 2017-10-18T13:49:30Z

.php_cs

@@ -15,7 +15,7 @@ return PhpCsFixer\Config::create()
        '@Symfony:risky' => true,
        '@PHP70Migration' => true,
        '@PHP71Migration' => true,
-        'array_syntax' => array('syntax' => 'short'),
+        'array_syntax' => ['syntax' => 'short'],


This one is ironic. 😭

dkarlovi · 2017-10-19T09:22:51Z

@sstok I've added some changes to the query generator itself:

getQuery now wraps in query so you can use it as is (refactored your tests to match)
implemented a registerField in some manner and refactored the hardcoded mappings in query generator and (your tests)

Can you take a look and point out any glaring errors or do you feel this might be a way to go?

dkarlovi · 2017-10-19T15:12:41Z

@sstok

I've taken the McDonald's principle and have made a property notation format proposition. If you have any feedback on this or anything else in this PR, I'd really love to hear it. Also need a review regarding normalizers (specifically date, but others too).

I think this has quite some potential, need to work out the details.

sstok

Amazing work so far 👍 I left some notes and idea's.
I'm away from keyboard later today. So may not be able to respond directly.

sstok · 2017-10-20T09:02:27Z

src/QueryConditionGenerator.php

    private $searchCondition;
-    private $fieldSet;
+    // private $fieldSet;


This property is used in the code?

Yep, my bad. :(

sstok · 2017-10-20T09:24:45Z

src/QueryConditionGenerator.php

+                    // https://www.elastic.co/guide/en/elasticsearch/reference/current/query-dsl-range-query.html#ranges-on-dates
+                    $values = array_values($valuesBag->getSimpleValues());
+                    foreach ($values as $idx => $value) {
+                        $valuesBag->removeSimpleValue($idx);


Note a ValuesBag is not immutable, and the QueryConditionGenerator should not change the ValuesBag.
Better to clone this.

For the future: Usually when a Query generator needs something special is to use TypeExtensions with options like elastic_search_structure_transformer which receives the ValuesBag (or ValuesGroup, or both) and allows to change it's structure.

Could you write/link an example? How do I register transformers per type (say, always transform Date like this, no matter where it is)?

I'm just working on this and it's annoying as it is, especially since it's only placeholder code.

I did something similar for the Doctrine DBAL integration 😋 query conversions.
https://github.com/rollerworks/search-doctrine-dbal/blob/master/src/Query/QueryField.php#L96

For example the https://github.com/rollerworks/search-doctrine-dbal/blob/master/src/Extension/Conversion/AgeDateConversion.php allows to search with both an age and a date.

sstok · 2017-10-20T09:28:12Z

src/QueryConditionGenerator.php

+                foreach ($valuesBag->get(Range::class) as $range) {
+                    $ids = array_merge($ids, range($range->getLower(), $range->getUpper()));
+                }
+                if (false === empty($ids)) {


No need for the explicit Boolean check, you can simple use if (f!empty($ids)) {
Same goes for all other code occurrences.

sstok · 2017-10-20T09:34:35Z

src/QueryConditionGenerator.php


-                $bool[$includingType][] = [$this->mappings[$fieldName]->indexName => $rangeParams];
-            }
+            if (false === $isId) {


This check needs to be done sooner as simple values can also apply to the id, no?

Yes, I'll be moving the whole thing.

sstok · 2017-10-20T13:11:40Z

src/QueryConditionGenerator.php

+        return $converter->convertValue($value);
+    }
+
+    /**


This phpdoc is useless, with private methods we only use it when it not directly clear what the method does or when there are some special rules/conventions. And thanks to strict types we don't need it here :)

TBH I'm not a big fan of the selective decision process if a docblock is necessary. It puts burden on the developer / reviewer to figure out. My path usually is to add them everywhere, it doesn't hurt readability, but it does allow for my tooling to work better.

I'll remove it here, obviously. 👍

sstok · 2017-10-20T13:13:46Z

src/ValueConversion.php

+namespace Rollerworks\Component\Search\Elasticsearch;
+
+/**
+ * Interface ValueConversion.


Please try to use a better description here. And don't forget to add an author tag 👍 your doing a great job here 😃

Thanks. I think I have a good idea how we can push the functionality further. Basically, special cases can go to the per-type extension which should allow, for example, exceptions for dates and IDs while keeping the basic query generator quite simple.

It'll be probably done monday. How do you plan actually implementing the client, if not using Elastica?

dkarlovi · 2017-10-23T09:52:33Z

Added a query conversion concept (which should handle special cases for IDs, dates, etc).

Also, you've inverted less than / greater than in your code and tests, I had a WTF moment there for a bit. 😭 Check the latest changes and feedback if you get the chance.

BTW how do I explain to the system that any date-y field should be converted by my DateConverter? Currently, BirthdayType is broken as it's not handled.

It's used not only by the query conversion process

dkarlovi · 2017-10-23T15:05:40Z

I've added support for nested queries (for example, binding property items[].label will do a nested query on items and do a regular query on items.label), will finish compare and matchers tomorrow.

What can we do to get this merged and released tomorrow? I'd like to include a preview version of this in my app ASAP (that's the reason I've done this work in the first place). If you could help me out here, it would be great. For example, start by submitting the package to Packagist so I can add my fork to my app.

Thanks.

sstok · 2017-10-24T07:50:55Z

Amazing work! This looks good enough to move forwards.

About the Client part, I didn't want to force a specific client implementation to use the user.
For Elastica we can pass the generated query to https://github.com/ruflin/Elastica/blob/master/lib/Elastica/Query.php 👍

For the API-Platform we need a full integration (using Elastica) to also make the mapping of Resources to Indexes/Documents work. But I'm not sure what the best approach is for this.
For limiting access we need to have some Context Listener that will (based on a custom implementation) ensures the the ElasticSearch query limits the results to what's accessible by the user.

dkarlovi · 2017-10-24T07:57:33Z

Amazing work! This looks good enough to move forwards.

\o/ Could you submit the package to Packagist, it would allow us to move forward on this.

For the API-Platform we need a full integration (using Elastica) to also make the mapping of Resources to Indexes/Documents work. But I'm not sure what the best approach is for this.

Maybe require FOS Elastica Bundle and compiler-pass the mapping into it?

sstok · 2017-10-24T09:05:09Z

I'm going to merge this one so we can move forward 👍

sstok · 2017-10-24T09:06:10Z

Thank you @dkarlovi

dkarlovi · 2017-10-24T09:06:43Z

Wait, I'm pushing more stuff!

dkarlovi · 2017-10-24T09:09:25Z

OK, opening a new PR. :)

sstok · 2017-10-24T09:23:45Z

No problem ;) I can even give you push access to this repository as you did an amazing job and I don't have always time to review everything in detail 😄

Basically I have a few rules (I still need to formalize these):

Don't use the GitHub UI for making direct commits (except for fixing small typo's) - Use pull requests with clear commit messages and use Git rebase (don't merge) for updating your local branch, be sure to clean-up any WIP commits afterwards (I didn't do this for this pull request as it would be to much work with set-up of the master branch and all).
Use https://github.com/park-manager/hubkit for merging pull-requests and making releases (no major releases yet!!)
Don't make a make major release (everything for RollerworksSearch v2 is still in alpha atm). Feel free to make unstable 0.x minor/patch releases (using Hubkit), note that you need to have git signing enabled for this.

When in doubt about something feel free to ask me 👍 (I will always do a review afterwards)
Again thank you every much for your amazing work and trust in RollerworksSearch 😸

dkarlovi · 2017-10-24T10:32:49Z

I can even give you push access to this repository

As said before, I really think you should consider a mono-repo for this project, the overhead of setup required for every single-repo will be quite high. Not that I'm requesting push access for that mono repo. :)

For example, I like to setup all the infra stuff to be run from a Makefile and then can just run make before commiting.

clear commit messages

That's why I'd rather you undo this merge as I've been doing WIP commits which I intended to fix afterward.

This PR was merged into the 1.0-dev branch. Discussion ---------- Missed in #1. Commits ------- a1d74a3 WIP pattern matcher support

dkarlovi force-pushed the functional-tests branch from bacfdde to 7728c19 Compare October 18, 2017 07:17

sstok reviewed Oct 18, 2017

View reviewed changes

dkarlovi commented Oct 18, 2017

View reviewed changes

Dalibor Karlović added 3 commits October 18, 2017 15:27

infra: switch the build process to a Docker-based one (including Travis)

68b72b7

infra: tweak PHP-CS-Fixer, add PHPStan config

8e45271

tests: initial functional tests

2e857c2

Backed by Elasticsearch 5.6

dkarlovi force-pushed the functional-tests branch from 30b2b28 to 2e857c2 Compare October 18, 2017 13:29

dkarlovi commented Oct 18, 2017

View reviewed changes

WIP

13c97be

Dalibor Karlović added 3 commits October 19, 2017 13:47

Range ID lookup

f8eb31a

Date lookup

dbaad39

WIP nested query

76dfbdb

sstok requested changes Oct 20, 2017

View reviewed changes

sstok added Feature Status: Needs Work labels Oct 20, 2017

WIP with conversions (broken currently)

731dd0d

sstok requested changes Oct 20, 2017

View reviewed changes

WIP with conversions

beffb03

Dalibor Karlović added 4 commits October 23, 2017 13:11

WIP with conversions

0a8c01d

WIP MoneyType

c0b53c2

WIP range support

1698882

WIP range support

760c48e

Dalibor Karlović added 2 commits October 23, 2017 14:58

WIP value conversion <-> query conversion cleanup

21445cc

WIP rename QueryConversionHints to QueryPreparationHints

3b94110

It's used not only by the query conversion process

dkarlovi force-pushed the functional-tests branch from 9f28f22 to 3b94110 Compare October 23, 2017 13:10

Dalibor Karlović added 2 commits October 23, 2017 15:18

WIP PHPStan fixes

dfdb919

WIP nested queries support

5c9a6f7

sstok approved these changes Oct 24, 2017

View reviewed changes

sstok merged commit ee51264 into rollerworks:implementation Oct 24, 2017

dkarlovi mentioned this pull request Oct 24, 2017

Pattern matcher support #2

Merged

sstok added a commit that referenced this pull request Oct 25, 2017

feature #2 Pattern matcher support (dkarlovi)

dbae0b8

This PR was merged into the 1.0-dev branch. Discussion ---------- Missed in #1. Commits ------- a1d74a3 WIP pattern matcher support

Add initial functional tests #1

Add initial functional tests #1

Conversation

dkarlovi commented Oct 17, 2017 • edited Loading

dkarlovi commented Oct 17, 2017

sstok Oct 18, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sstok commented Oct 18, 2017

dkarlovi commented Oct 18, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sstok commented Oct 18, 2017

dkarlovi commented Oct 18, 2017

Choose a reason for hiding this comment

dkarlovi commented Oct 19, 2017

dkarlovi commented Oct 19, 2017

sstok left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dkarlovi commented Oct 23, 2017

dkarlovi commented Oct 23, 2017 • edited Loading

sstok commented Oct 24, 2017

dkarlovi commented Oct 24, 2017

sstok commented Oct 24, 2017

sstok commented Oct 24, 2017

dkarlovi commented Oct 24, 2017

dkarlovi commented Oct 24, 2017

sstok commented Oct 24, 2017

dkarlovi commented Oct 24, 2017

dkarlovi commented Oct 17, 2017 •

edited

Loading

sstok Oct 18, 2017 •

edited

Loading

dkarlovi commented Oct 18, 2017 •

edited

Loading

dkarlovi commented Oct 23, 2017 •

edited

Loading