hkulekci / qdrant-php

Qdrant is a vector similarity engine & vector database. It deploys as an API service providing search for the nearest high-dimensional vectors. With Qdrant, embeddings or neural network encoders can be turned into full-fledged applications for matching, searching, recommending, and much more!
MIT License
93 stars 21 forks source link

Add getters for structs. #21

Closed gregpriday closed 1 year ago

gregpriday commented 1 year ago

I've found a few instances where I needed to get variable values from structs. Namely MultiVectorStruct, PointsStruct, PointStruct, VectorStruct. A very simple solution would be to make these variables public, but a more maintainable solution might be to have a single trait for all structs that allows get access to all protected variables.

I haven't checked this implementation, but something like this:

<?php
namespace Qdrant\Traits;

use InvalidArgumentException;
use ReflectionProperty;

/**
 * Trait ProtectedPropertyAccessor
 *
 * Allows access to protected properties through the magic __get method.
 */
trait ProtectedPropertyAccessor
{
    /**
     * Magic method to implement generic getter functionality for protected properties.
     *
     * @param string $property The name of the property to get.
     * @return mixed The value of the property.
     * @throws InvalidArgumentException if the property doesn't exist or is not protected.
     */
    public function __get(string $property)
    {
        if (property_exists($this, $property)) {
            $reflection = new ReflectionProperty($this, $property);
            if ($reflection->isProtected()) {
                return $this->$property;
            } else {
                throw new InvalidArgumentException("Access to property '$property' is not allowed");
            }
        }

        throw new InvalidArgumentException("Property '$property' does not exist");
    }
}

What do you think @hkulekci?

gregpriday commented 1 year ago

Here's the PR if you're interested in adding this: https://github.com/hkulekci/qdrant-php/pull/22

hkulekci commented 1 year ago

Could you please share a little snippet to see the problem to see why we need this type of trick?

hkulekci commented 1 year ago

It is a good opinion to be able to reach protected fields but I could not get the real point of what we are trying to solve exactly. And also, we need to think about testability. We just try to remove getter methods from the Struct classes, right?

gregpriday commented 1 year ago

Sorry, I just spent some time looking through my code and I remembered I actually needed this in SearchRequest, not the Structs. I can change the PR I opened.

I needed it to implement pagination. I need to know a bit about the SearchRequest to get the total results count, here https://github.com/gregpriday/laravel-scout-qdrant/blob/develop/src/Scout/QdrantScoutEngine.php#L268 - I still need to finish that function so it uses Qdrant's core count feature when possible.

My workaround was to pass these variables here https://github.com/gregpriday/laravel-scout-qdrant/blob/develop/src/Scout/QdrantScoutEngine.php#L205-L209, but I'd much rather rely on the SearchRequest as the single source of truth.

Overall though, making these variables accessible can only help other developers integrate with Qdrant. To quote the Python guidelines, "we are all consenting adults here".

In terms of testing, we can add a simple test for the trait that it allows access to protected variables and blocks access to private variables.

hkulekci commented 1 year ago

I think we are done with this, right? Please feel free to close it if it is. @gregpriday

gregpriday commented 1 year ago

All done, yes. Sorry, just saw this now.