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

Fix ES exception when a field missed in using updateAllCounters. #334

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

jafaripur
Copy link
Contributor

@jafaripur jafaripur commented Jun 3, 2023

Q A
Is bugfix? ✔️
New feature?
Breaks BC?

When using updateAllCounters for a field which is missed or is null get exception error from ElasticSearch:

Cannot invoke "Object.getClass()" because "left" is null

This behavior should cast null and missed field to 0.

@what-the-diff
Copy link

what-the-diff bot commented Jun 3, 2023

PR Summary

  • Deprecated method removal
    The method setPrimaryKey() has been marked as deprecated and is now removed.
  • Null check added in updateAllCounters()
    To prevent NullPointerExceptions, a null check has been added when updating counters in the Elasticsearch indexing engine.

ActiveRecord.php Outdated Show resolved Hide resolved
@samdark samdark added this to the 2.1.5 milestone Jun 4, 2023
@beowulfenator
Copy link
Collaborator

@jafaripur Thank you for your improvement. This is clearly a good change. However, let's explore all possibilities and perhaps make this function even better. As far as I know, the different values for an attribute are possible. Here is my suggestion on handling them all:

Value Action
A null value Cast to integer and make it 0, then update
A "number" value Update normally
A non-number value Raise exception
An array with a single value Look at the single value and act accordingly. Preserve the array, so with a +1 increment, [123] becomes [124], not 124
An array with multiple values Raise exception

If possible, the exception should be meaningful, so it should be obvious that we are updating a value that can not be updated.

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.

None yet

3 participants