meilisearch / integration-guides

Central reference for Meilisearch integrations.
https://meilisearch.com
MIT License
137 stars 15 forks source link

Avoid mismatch between Meilisearch server and the SDK #264

Closed brunoocasali closed 1 year ago

brunoocasali commented 1 year ago

Context: Today, when a user updates its SDK and uses a new method, suppose multiSearch, it also should update the Meilisearch server running to the latest version. Otherwise, they will face an error. This situation is terrible for the user because they will see that probably only in production, bringing a lot of headaches, hours of debugging, and more.

So, knowing that I suggest to the @meilisearch/integration-team think and propose an active solution to solve that issue. Alerts, documentation anything that can help are welcome. A solution proposal was made here, in the rails repository and could be a starting point for the discussion.

TODO:

@meilisearch/integration-team, we should change our release disclaimer to this:


This version introduces features released on Meilisearch v1.1.0 :tada: Check out the changelog of Meilisearch v1.1.0 for more information on the changes. If you want to adopt new features of this release, update the Meilisearch server to the according version.


So this way, we state clearly to the users that if they have plans to adopt the new features, they MUST update the Meilisearch server as well, so we can at least try to avoid situations like this:

And the list goes on. There are a lot of examples, and they will still happen even after Meilisearch v1.

brunoocasali commented 1 year ago

📡 to @gmourier since we're talking about the community feedback from the integrations, this one is clearly (for me) the most impactful one.

sanders41 commented 1 year ago

I think with it in the README it won't solve the problem. I highly doubt people are reading the full readme before using the client, especially if it is an update to the client. Unfortunately I don't have a suggestion for a better solution though.

brunoocasali commented 1 year ago

Yeah, I agree with you 😢 . I want something that could raise some exceptions or at least a warning that clarifies to the user what is happening.

sanders41 commented 1 year ago

The challenge there is going to be Meilisearch is going to return a generic error because by definition of the problem the server won't know what you are trying to do. Only the client could possibly know the issue so my only thought right now (and I don't think it is a good one) is the client could request the server version and use that to throw an error if something is used that isn't implemented in the server version.

sanders41 commented 1 year ago

Maybe catch generic exception, then request the version number, if it is a version mismatch raise a version exception, or the original exception of not? I think maintenance with this could be difficult though.

brunoocasali commented 1 year ago

That was exactly what I was thinking about (your first message), but I don't know how it could look in the code, the overhead about making a /version request, and the /version has to be authenticated today.

sanders41 commented 1 year ago

Here is a VERY rough version of something that may work. Obviously this is Python and I haven't though much about what an equivalent implementation in other languages would look like.

class VersionError(Exception):
    ...

def check_version(min_version: str) -> Callable:
    def decorator_check_version(func: Callable) -> Callable:
        @wraps(func)
        def wrapper(self: Union["Client", "Index"], *args: Any, **kwargs: Any) -> Any:
            try:
                return func(self, *args, **kwargs)
            except MeilisearchApiError:
                server_version = self.http.get(self.config.paths.version)
                split_version = [int(x) for x in server_version["pkgVersion"].split(".")]
                split_min = [int(x) for x in min_version.split(".")]

                # this semver check is not correct, it’s just used as an example and would need to be corrected if used in the clients 
                if split_version[0] < split_min[0]:
                    v = self.http.get(self.config.paths.version)
                    raise VersionError(v)

                if split_version[1] < split_min[1]:
                    raise VersionError(
                        f"{func.__name__} is only supported in Meilisearch >= {server_version['pkgVersion']}"
                    )

                if split_version[2] < split_min[2]:
                    v = self.http.get(self.config.paths.version)
                    raise VersionError(v)

                raise

        return wrapper

    return decorator_check_version

class Client:
    ...

    @check_version(min_version="1.2.0")
    def shiny_new_feature(self) -> None:
        ...
bidoubiwa commented 1 year ago

the client could request the server version and use that to throw an error if something is used that isn't implemented in the server version.

The issue with that is when the API key provided does not have the right to call /version. On search keys or custom keys for example.

Another approach, way more annoying to maintain, could be to catch the error and analyze the error returned on all routes introducing a change between versions.

For example on multi-search, we can catch a 404, output a warning that multi-search only works with Meilisearch versions above 1.1, and then throw the original error.

On routes introducing new query parameters, we can catch them in the message error:

"message": "Unknown parameter `primaryKey`: expected one of `offset`, `limit`, `fields`",

Again, we can throw an error on for each unknown parameter provide a warning with the version in which it is introduced before throwing the original error.

By throwing a warning, we only suggest the error might come from there, thus we do not exclude the possibility that the error has another cause.

sanders41 commented 1 year ago

The issue with that is when the API key provided does not have the right to call /version. On search keys or custom keys for example.

I didn't think about this. This means the client is going to have to 100% manage all of this itself and maintain differences between package versions at runtime. This is going to be really hard to keep up with.

brunoocasali commented 1 year ago

I don't think it is doable in the long-run @bidoubiwa. It will be incredibly painful to handle those differences for future versions, also we don't know when we will have a v2.

The idea proposed by @sanders41 could work if we could make /version publically available.

ahmednfwela commented 1 year ago

what if we add a new field to the exception object containing meili server version ?

then we define a field in meili search client (in all sdks) that contain the min supported version

sanders41 commented 1 year ago

This will go back to the issue @bidoubiwa pointed out. It is possible that the token can't access the version from the server so the client doesn't have a way to get this information.

An additional complication here is it is possible that each individual method can have a different minimum supported version. So there needs to be some way to specify that for each thing you can do. Or if you want to specify a minimum version for everything you also have to give a maximum version.

ahmednfwela commented 1 year ago

@sanders41 I think it should just get the version anyway regardless of api key, since it's harmless information

we can replace the static field with something like

ensureMinVersion(minFeatureVersion, errorVersion)

sanders41 commented 1 year ago

I don't disagree. It's just that it's not currently possible.

brunoocasali commented 1 year ago

I'll forward this issue to the product team, so it can be prioritized :)