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

[Elasticsearch] contextual support #251

Merged
merged 3 commits into from
Dec 12, 2018
Merged

Conversation

dkarlovi
Copy link
Contributor

@dkarlovi dkarlovi commented Nov 28, 2018

Q A
Bug fix? no
New feature? yes
BC breaks? yes
Deprecations? no
Tests pass? yes
Fixed tickets #249
License MIT
Doc PR N/A

Closes #249. Solves cases 1) and 2) from there. Case 3) is handled by decorating the factory, all use cases work well in my app.

@dkarlovi dkarlovi changed the title [Elasticsearch] contextual support WIP [Elasticsearch] contextual support Nov 28, 2018
@dkarlovi
Copy link
Contributor Author

dkarlovi commented Nov 28, 2018

@sstok this is already solving points 1) and (almost) 2) from #249, do you have any objections about the direction? Note the ParameterBag addition in core.

@dkarlovi
Copy link
Contributor Author

BTW this is a snippet of my APIP mapping:

                <attribute name="_any">
                    <attribute name="elasticsearch">
                        <attribute name="mappings">

                           <!-- case 1) and 2) from #249 -->
                            <attribute name="origin">
                                <attribute name="property">/app_{locale}_article/article#origin.identifier</attribute>
                                <attribute name="conditions">
                                    <attribute>
                                        <attribute name="property">/app_{locale}_article/article#origin.client</attribute>
                                        <attribute name="value">{client_id}</attribute>
                                    </attribute>
                                </attribute>
                            </attribute>

                           <!-- to be case 3) from #249 (needs to be added at runtime) -->
                            <attribute name="att_note">
                                <attribute name="property">/app_{locale}_article/article#extended_attribute_bag_client_user>extended_attribute_bag_client_user.values_{client_id}.note</attribute>
                                <attribute name="conditions">
                                    <attribute>
                                        <attribute name="property">/app_{locale}_article/article#extended_attribute_bag_client_user>user</attribute>
                                        <attribute name="value">{user_id}</attribute>
                                    </attribute>
                                </attribute>
                            </attribute>
                        </attribute>
                </attribute>
            </attribute>

@dkarlovi dkarlovi force-pushed the context branch 2 times, most recently from 3744382 to 210160d Compare November 28, 2018 16:01
@dkarlovi
Copy link
Contributor Author

dkarlovi commented Nov 28, 2018

Remaining:
1. "unpack" the conditions supplied with the mapping and apply them too if non-empty
2. figure out how to create a mapping dynamically (this will probably be tricky) (decorate the factory)

@dkarlovi
Copy link
Contributor Author

dkarlovi commented Dec 6, 2018

@sstok this works for contextual conditions already, you can see both runtime properties and contextual conditions being assigned.

The only thing that comes to mind might be: currently it only allows for simple comparison with AND in contextual conditions, which might be too restrictive, but it also might be fine (so it behaves like AND additional_field = :value in SQL).

IMO we could clean up this PR and merge here, adding runtime mappings could be a separate PR.

WDYT?

@dkarlovi dkarlovi changed the title WIP [Elasticsearch] contextual support [Elasticsearch] contextual support Dec 7, 2018
@dkarlovi
Copy link
Contributor Author

dkarlovi commented Dec 7, 2018

This works as-is, ready for review.

@dkarlovi dkarlovi force-pushed the context branch 2 times, most recently from eae3a8d to 7cec171 Compare December 12, 2018 09:51
@dkarlovi
Copy link
Contributor Author

Yay, all green! \o/

@sstok do you think you'd have time to review?

@sstok
Copy link
Member

sstok commented Dec 12, 2018

Looks good to me, are you going to continue in this pull request or in a follow-up?
And some tests would be nice 😊

@dkarlovi
Copy link
Contributor Author

are you going to continue in this pull request or in a follow-up?

Not really much to follow-up except #252, but I think that's fine as a separate PR, since it would require some discussion.

some tests would be nice

Adding now. 👍

@dkarlovi dkarlovi force-pushed the context branch 2 times, most recently from 6b2bf3f to 8948816 Compare December 12, 2018 12:22
@dkarlovi
Copy link
Contributor Author

This is why reviews are a thing. 🙈

I was sure I added the whole missing part of functionality in another PR before so "it's OK tests are missing here, they're already $there". But there is no other PR, just a different checkout with a whole set of changes which weren't pushed.

@dkarlovi
Copy link
Contributor Author

@sstok it should be ready now, tests added from the other branch and it all works here and in my app.

@sstok
Copy link
Member

sstok commented Dec 12, 2018

Can I merge? 👍

@dkarlovi
Copy link
Contributor Author

Yup! \o/

@sstok sstok merged commit 1f12839 into rollerworks:master Dec 12, 2018
@sstok
Copy link
Member

sstok commented Dec 12, 2018

Thank you @dkarlovi

@dkarlovi dkarlovi deleted the context branch December 12, 2018 15:13
@dkarlovi
Copy link
Contributor Author

Thank you for the merge. 👍

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

Successfully merging this pull request may close these issues.

Contextual / runtime params support
2 participants