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

after() should accept a raw "lastEvaluatedKey" value #127

Closed
zoul0813 opened this issue Feb 1, 2018 · 5 comments
Closed

after() should accept a raw "lastEvaluatedKey" value #127

zoul0813 opened this issue Feb 1, 2018 · 5 comments

Comments

@zoul0813
Copy link
Contributor

zoul0813 commented Feb 1, 2018

Sometimes, you may not have an instance of a model available (for example, when paginating server side or using scripts) and instead only have the "lastEvaluatedKey" available. It seems inefficient to use this key to query for a single model to pass to after(). Instead, I propose that after() supports accepting a DynamoDbModel to maintain the current after() functionality but also allowing you to just set lastEvaluatedKey to whatever was passed in if it is not a DynamoDbModel.

Something like this, where $after could be the value of $model->getLastEvaluatedKey()

public function after($after = null)
{
    $afterKey = $after;
    if ($after instanceof DynamoDbModel) {
        if (empty($after)) {
            $this->lastEvaluatedKey = null;

            return $this;
        }

        $afterKey = $after->getKeys();

        if ($index = $this->conditionsContainIndexKey()) {
            $columns = array_values($index['keysInfo']);
            foreach ($columns as $column) {
                $afterKey[$column] = $after->getAttribute($column);
            }
        }
    }

    $this->lastEvaluatedKey = $this->getDynamoDbKey($afterKey);

    return $this;
}
@baopham
Copy link
Owner

baopham commented Feb 1, 2018

How about add a afterKey()? One method doing one thing is preferred.

@zoul0813
Copy link
Contributor Author

zoul0813 commented Feb 1, 2018

That would work.

Could we also add in getLastEvaluatedKey() and setLastEvaluatedKey($key) methods ... I seem to recall these existing at some point, but that may have only been in my local fork. This would allow you to retrieve the current lastEvaluatedKey (as it's currently protected) on the query.

I can make these edits and create a PR before end of day, if you'd like.

@baopham
Copy link
Owner

baopham commented Feb 1, 2018

afterKey would then be the same as setLastEvaluatedKey. As for the getter, could you give me your use case? Maybe we can find a more user friendly way. I don't want the user to have to be aware of the query builder too much except of their own model.

@zoul0813
Copy link
Contributor Author

zoul0813 commented Feb 1, 2018

Except that afterKey returns an instance of the QueryBuilder for chaining, and get/setLastEvaluatedKy are just methods for accessing the value.

As for use case, it's mainly for testing/debugging on my end to make sure that the correct values are being set, etc. It doesn't need to be a documented method, and could be for internal use only... any references to $this->lastEvaluatedKey could use getLastEvaluatedKey which could eventually have some validation logic in it... especially if we allow the user to set the key manually.

@baopham
Copy link
Owner

baopham commented Feb 1, 2018

Let me think about it, my immediate thought is why not use getDynamoDbQuery but probably it doesn't have the LastEvaluatedKey. I can't remember.
In the mean time, you can create a PR for afterKey if that is blocking you.

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

No branches or pull requests

2 participants