\RavenDB\Documents\Commands\GetDocumentsCommand::withStartWith() throw on not null but empty prefix

[Geolim4]

Hello,

Method \RavenDB\Documents\Commands\GetDocumentsCommand::withStartWith throw when $startWith is an empty string (but is not null), while an empty string is working and theoretically a valid prefix.

Solution:

Replace:

        if (empty($startWith)) {
            throw new IllegalArgumentException("startWith cannot be null");
        }

by:

        if ($startWith === null) {
            throw new IllegalArgumentException("startWith cannot be null");
        }

[ayende]

Can you explain what your scenario is for using startsWith with an empty string? Are you trying to iterate over all the documents?

[Geolim4]

Can you explain what your scenario is for using startsWith with an empty string? Are you trying to iterate over all the documents?

Yes indeed, with a limit of 1000 documents ( via the pageSize parameter), there's ofc an option pattern I'm using for "matches" parameters, but it's effectively what I'm doing.

[Geolim4]

But if I'm going on the wrong way, just tell me what alternative to use :P

I'm only looking for a way to fetch up to 1000 documents (metadata only, I don't need the rest) with an optional "matches" parameter.

[ayende]

Can you go back a step? What is the actual scenario? You want to find 1000 documents based on their id?

Note that if you matches for empty startsWith means that you are doing a full scan.

Basically:

for item in docs:
   if match(item):
       yield item

I assume that this isn't what you want.

[Geolim4]

Not exactly, I have a public API that allow users to retrieve all the cache items (the objects stored in Ravendb) with a maximum hard-limit of 999 items. This public API have an optional "$pattern" parameter that I successfully bound to Ravendb:

    /**
     * @return array<int, string>
     * @throws PhpfastcacheDriverException
     * @throws PhpfastcacheInvalidArgumentException
     */
    protected function driverReadAllKeys(string $pattern = ''): iterable
    {
        $keys = [];
        if (empty($this->documentPrefix)) {
            throw new PhpfastcacheUnsupportedMethodException('A document prefix must be configured and not empty to be able to load all items from Raven.');
        }
        $results = $this->instance->loadStartingWith(
            RavenProxy::class,
            $this->documentPrefix,
            $pattern,
            0,
            ExtendedCacheItemPoolInterface::MAX_ALL_KEYS_COUNT
        );

        if (is_iterable($results)) {
            /** @var RavenProxy $item */
            foreach (iterator_to_array($results) as $item) {
                $keys[] = $item->getKey();
            }
        }

        return $keys;
    }

So to bypass this "error", I made the prefix mandatory so the /docs can be called without errors.

[ayende]

What is the meaning of the pattern in your case? What is the user's scenario for this?

[Geolim4]

It is like a "I want all the cache item that contain xxxx keyword".

And I wrap the keyword with * like this: *keyword*. it is currently really working well:

But the pattern is working well, it's just the $startsWith prefix which is mandatory on the Php client while it is not on the Ravendb API. Well not exactly "mandatory" but incorrectly checked as it considers an empty string as null due to if (empty($startWith)) check :)

[ayende]

The issue is that this works as long as you have a small amount of data. But if you have 1M items, you may need to scan through all of those. What kind of freedom do you have in this scenario? If I was writing independently, I would want to do something like:

{ data: ..., id: ..., tags: ["cache-test3"] }

So add the ability to tag a cache item (with multiple tags), then allow to query on that. This will result in both better API and use case, I think. But I don't know if this is something that is possible from your end given that you integrate with an existing project

[Geolim4]

Tags already exists and are handled well on my side, but don't bother about the pattern, it is not the issue here 😂 The issue is just that the php client requires developers to provide an non-empty string to $startWith while it is not required at all by the API. Because it considers an empty string equal to null.

So; short version: I'd like to be all to grab the 999 first documents without having to provide $startWith or $matches ☺️

[ayende]

That is indeed a bug, and we can fix that. @alxsabo

The actual use case is probably better off with a query, rather that loadStartingWith

[alxsabo]

Ok. I will make a bug fix for this issue.

[Geolim4]

Aside topic:

Do the ravendb client is Semver-compliant ?

Because I found changes that are completely breaking between 5.21 et 5.2.6 :(

[ayende]

There shouldn't be any breaking changes, only additional APIs

[alxsabo]

Because I found changes that are completely breaking between 5.21 et 5.2.6 :(

Can you tell us what changes you found?

[Geolim4]

PHP version changes from 8.0 to 8.1 minimum in between 2 PATCH version :(

So basically when my CI is running, composer rollback from 5.2.6 to 5.2.1 to run the tests which make new methods added between 5.2.1 and 5.2.6 suddenly unavailable :(

Because Phpfastcache runs from 8.0 up to 8.4, so the Github Actions runs the tests on each of them.

[alxsabo]

Yes, we update minimum version of PHP to be 8.1, and RavenDB cant be run on PHP 8.0

[Geolim4]

Yes, that was a breaking changes, for being Semver compliant this change should have occurred on a MINOR or MAJOR version. Here you increased the minimum support between 2 PATCH version, so users from the 5.2 library will have incompatible code between php 8.0 and 8.1. Fine for me, I'll make the necessary checks and changes, but you should be carefully not introducing too much BC else users will have pain maintaining their code to your client :)

[alxsabo]

Thank you for raising this concern. We'll ensure better backward compatibility in our future updates.

[Geolim4]

This is something I'm very vigilant to since I took the Phpfastcache library 8 years ago. PHP version was only increased from major to major version to avoid users to have brutal composer rollbacks and versions constraint sticked.

I will continue my implementation on my side until I consider it stable enough to make an official release of Ravendb support on Phpfastcache. Thanks for the quick support however !