[MODEL] Do not override existing methods #936
Conversation
|
💚 CLA has been signed |
|
Agreement review in progress, just wanted to put up this PR in the meantime |
|
A rep from my company signed the CLA (via Docusign), and included my name & Github handle in the company agreement. I think the bot doesn't take that into account, hence the failure @picandocodigo (tagging you because you recently released a new version of this gem), do you think you could help me route this PR to the right folks for review? I appreciate it! |
|
Hi @michaelmcchen, thanks for this contribution! I'll take a look at it |
|
Hi, we're in the process of changing our codebase so we can upgrade our clusters to ES 7x and we've stumbled on this as well. Can we expect this PR to be merged? Also to address @michaelmcchen 's concern, I think this behavior should probably have some unit tests to help avoid any regressions :/ |
|
I'll bump the priority on this issue and let you know about a future release 👍 |
|
@picandocodigo thank you very much! If you'd like we could maybe try to write some tests too! |
|
@greenonion definitely, some tests would really help. Thanks! |
michaelmcchen
force-pushed
the
undo-es-models-priority
branch
from
affa0cb to
a2c2950
Compare
Agreed! I've updated an existing spec to cover this case and tested it out by reverting my original commit (which now fails the spec). @greenonion @picandocodigo |
| @@ -48,6 +48,7 @@ def self.search(query, options={}) | |||
| end | |||
|
|
|||
| DummyIncludingModel.__send__ :include, Elasticsearch::Model | |||
| DummyIncludingModelWithSearchMethodDefined.__send__ :include, Elasticsearch::Model | |||
There was a problem hiding this comment.
Ah so there was a test for this (line 71) but it wasn't actually set up correctly. Nice!
Fix issue elastic#924 more: elastic#936
Fix issue elastic#924 more: elastic#936
This PR reverts the line from this commit: https://github.com/elastic/elasticsearch-rails/pull/893/files to address the issue described here #924
My biggest concern is that this is a very subtle change in behavior that could lead to bugs, especially in a project where custom methods with similar names (import, search, etc.) are defined. It seems safer to keep the existing behavior (pre 7.x.x elasticsearch-rails) and force users who are facing this sort of conflict to be more explicit about their method choice (via
ModelName.__elasticsearch__.method())