Make some fields in *Stats structs optional to support older servers
#690
Conversation
WalkthroughField types in the Changes
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~3 minutes Possibly related PRs
Poem
Note ⚡️ Unit Test Generation is now available in beta!Learn more here, or try it out under "Finishing Touches" below. 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
✨ Finishing Touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/indexes.rs (1)
1903-1908: Add#[serde(default)]to make deserialization from pre-1.13 servers rock-solid
Option<T>already handles missing JSON keys, but adding an explicit#[serde(default)]expresses that intention in the type definition and protects you from a future scenario where the server explicitly returns"numberOfEmbeddedDocuments": null(which would otherwise still work) and adeny_unknown_fieldsdeserialization context (which would unexpectedly fail when the key is absent).
It also makes the intent clearer to downstream readers.-/// Is `None` for Meilisearch servers older than 1.13. -pub number_of_embedded_documents: Option<usize>, +/// Is `None` for Meilisearch servers older than 1.13. +#[serde(default)] +pub number_of_embedded_documents: Option<usize>, ... -/// Is `None` for Meilisearch servers older than 1.13. -pub number_of_embeddings: Option<usize>, +/// Is `None` for Meilisearch servers older than 1.13. +#[serde(default)] +pub number_of_embeddings: Option<usize>,Minor, non-blocking.
(Remember to bump the crate’s semver major/minor as this is an API break for consumers.)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/client.rs(1 hunks)src/indexes.rs(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: integration-tests
🔇 Additional comments (1)
src/client.rs (1)
1155-1158: No downstream usages ofused_database_sizefound—no client code changes requiredA global search for
used_database_sizeonly returns its declaration insrc/client.rs(line 1157) with no other call-sites to update. Since there are no places treating this field as a plainusize, no additional pattern-matching or unwrapping adjustments are needed.
These three fields were added in 1.13. If you want to support older servers, these fields need to be optional. https://github.com/meilisearch/meilisearch/releases/tag/v1.13.0
LukasKalbertodt
force-pushed
the
stats-for-older-servers
branch
from
1ae5cc2 to
6ba586e
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #690 +/- ##
=======================================
Coverage 85.76% 85.76%
=======================================
Files 19 19
Lines 5860 5860
=======================================
Hits 5026 5026
Misses 834 834 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
Hello, thank you for the PR.
Sorry, but we do not. This is only one of many incompatibilities and it would require way too much work. If you are the owner of the outdated meilisearch instance I suggest you upgrade to the latest version. If you can't, just downgrade this library. Cargo supports having the same library multiple times with different versions if you need to handle multiple meilisearch versions in the same program |
Can you explain more how this could work ? |
|
This probably needs a few changes to compile but it will give you an idea [dependencies]
meilisearch-sdk = { package = "meilisearch-sdk", version = "0.29" }
old-meilisearch-sdk = { package = "meilisearch-sdk", version = "0.28" }use meilisearch_sdk::Client;
use old_meilisearch_sdk::Client as OldClient;
let client = Client::new();
let old_client = OldClient::new(); |
Hu interesting, I didn't know that. You should probably adjust your README then. It says:
Which to me reads "all 1.x versions". I think it should explicitly say that only the last version is supported or something like that. Also, while I certainly understand the decision, it is a bit limiting for users. Especially since support for new features of Meili sometimes lands really late in this library. And when it finally lands, then the new (SDK) version might exclude many server versions, that's unfortunate. For example, the latest release of this library finally supports federated multi search (introduced in 1.10) and then immediately bumps the minimum supported version to 1.13. Just as context: users of our software host it and Meilisearch themselves. So bumping our Meilisearch version requirement means it's a breaking change and admins cannot easily upgrade our software, but have to upgrade Meilisearch as well. I don't expect to change your opinion on this, but just wanted to give you some insight why it's not optimal for us. |
|
I understand the confusion. I have checked and can confirm SDKs do not intend to support old versions. The readme is based on a template common to all SDKs so I can't clarify here right away. It already correctly links to the latest version though. I suggest you keep using a fork of this repository if you find a way to suit your use case. We do not plan to refactor anything big in the near future, so you will probably be able to keep our changes flowing to your fork quite easily |
These three fields were added in 1.13. If you want to support older servers, these fields need to be optional.
https://github.com/meilisearch/meilisearch/releases/tag/v1.13.0
Unfortunately, this is a breaking change for this library, which is unfortunate, as v0.29 not supporting older servers seems like a mistake to me. Should we solve this differently so that this can be released as a patch release? Or is it fine to just immediately release v0.30?
Summary by CodeRabbit
Documentation
New Features