-
Notifications
You must be signed in to change notification settings - Fork 102
Add support for webhook API #719
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
Conversation
WalkthroughAdds full webhook API support: new Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant User
participant Client
participant HttpClient
participant MeilisearchAPI
rect rgb(235,248,255)
Note over User,Client: Create webhook
User->>Client: create_webhook(WebhookCreate)
Client->>Client: serialize WebhookCreate
Client->>HttpClient: POST /webhooks
HttpClient->>MeilisearchAPI: POST /webhooks
MeilisearchAPI-->>HttpClient: 201 WebhookInfo
HttpClient-->>Client: WebhookInfo
Client-->>User: WebhookInfo
end
rect rgb(245,255,235)
Note over User,Client: List webhooks
User->>Client: get_webhooks()
Client->>HttpClient: GET /webhooks
HttpClient->>MeilisearchAPI: GET /webhooks
MeilisearchAPI-->>HttpClient: 200 WebhookList
HttpClient-->>Client: WebhookList
Client-->>User: WebhookList
end
rect rgb(255,245,235)
Note over User,Client: Update webhook (partial)
User->>Client: update_webhook(uuid, WebhookUpdate)
Client->>Client: serialize WebhookUpdate (HeadersUpdate state)
Client->>HttpClient: PATCH /webhooks/{uuid}
HttpClient->>MeilisearchAPI: PATCH /webhooks/{uuid}
MeilisearchAPI-->>HttpClient: 200 WebhookInfo
HttpClient-->>Client: WebhookInfo
Client-->>User: WebhookInfo
end
rect rgb(250,250,250)
Note over User,Client: Delete webhook
User->>Client: delete_webhook(uuid)
Client->>HttpClient: DELETE /webhooks/{uuid}
HttpClient->>MeilisearchAPI: DELETE /webhooks/{uuid}
MeilisearchAPI-->>HttpClient: 204 No Content
HttpClient-->>Client: ()
Client-->>User: ()
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Poem
Pre-merge checks and finishing touches✅ Passed checks (5 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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. Comment |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (6)
src/webhooks.rs (4)
33-41: Create payload API is clear; small naming nitBoth with_header(self) and insert_header(&mut self) are handy. Consider aligning names (e.g., with_header/with_header_mut) for symmetry, but optional.
Also applies to: 43-68
83-137: Update builder is practical; optional ergonomic variantCurrent &mut builder works. Optionally add a by‑value variant for chaining from new(), mirroring WebhookCreate.
impl WebhookUpdate { - pub fn new() -> Self { Self::default() } + pub fn new() -> Self { Self::default() } + #[must_use] + pub fn with_url_owned(mut self, url: impl Into<String>) -> Self { + self.url = Some(url.into()); + self + } }
139-167: Avoid serde_json dependency in Serialize impl (minor)You can emit null without importing serde_json by serializing an Option::None.
- HeadersUpdate::Reset => { - map.serialize_entry("headers", &Value::Null)?; - } + HeadersUpdate::Reset => { + let none: Option<()> = None; + map.serialize_entry("headers", &none)?; + }
199-241: Consider adding reset_headers() call to webhook_crud integration test for comprehensive e2e coverageThe
reset_headers()method exists (line 133, src/webhooks.rs) and correctly serializes to"headers": null, which per Meilisearch 1.17 API clears all headers server-side. While your current test validates set/remove operations, adding a step that callsreset_headers()and asserts headers are cleared would provide complete end-to-end coverage—especially since a unit test already validates the serialization (lines 193–196)..code-samples.meilisearch.yaml (1)
1959-1977: Samples look good; fully qualify types to avoid missing importsIf the docs runner doesn’t inject imports, qualify WebhookCreate/WebhookUpdate.
- let mut payload = WebhookCreate::new("WEBHOOK_TARGET_URL"); + let mut payload = meilisearch_sdk::webhooks::WebhookCreate::new("WEBHOOK_TARGET_URL"); @@ - let mut update = WebhookUpdate::new(); + let mut update = meilisearch_sdk::webhooks::WebhookUpdate::new();Please run the docs build to ensure these snippets compile where expected.
src/client.rs (1)
1221-1250: get_webhook by UUID: consider a Uuid overloadCurrent signature is fine. Optionally add a helper accepting &Uuid to avoid callers allocating strings.
pub async fn get_webhook_uuid(&self, id: &uuid::Uuid) -> Result<WebhookInfo, Error> { self.get_webhook(id.to_string()).await }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
.code-samples.meilisearch.yaml(1 hunks)Cargo.toml(2 hunks)src/client.rs(2 hunks)src/lib.rs(1 hunks)src/webhooks.rs(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/webhooks.rs (2)
src/client.rs (3)
new(54-66)None(1446-1446)None(1525-1525)meilisearch-test-macro/src/lib.rs (1)
meilisearch_test(14-201)
🔇 Additional comments (12)
Cargo.toml (1)
29-29: uuid + serde enablement looks correctMatches the new webhook models’ need to (de)serialize Uuid; wasm config keeps default-features off while enabling "js" + "serde". LGTM.
Also applies to: 40-40
src/webhooks.rs (5)
7-14: Webhook model: solid and idiomaticCorrect fields, defaults, and casing; ready for round‑trip with API.
16-24: Flattened WebhookInfo is appropriateFlatten avoids nested payloads and matches typical Meilisearch responses.
26-31: List wrapper matches GET /webhooksSimple and sufficient.
70-81: HeadersUpdate states are well chosenNotSet/Reset/Set cleanly encode API semantics.
176-197: Serialize tests cover set/remove/reset casesGood coverage for JSON shape.
src/lib.rs (1)
267-268: Module export is in the right placePublicly exposing webhooks alongside other top‑level modules is correct.
src/client.rs (5)
17-17: Importing webhook types: OKTypes are correctly surfaced from the new module.
1193-1220: get_webhooks aligns with conventionsEndpoint, method, and 200 status match expectations.
1252-1285: create_webhook: good status and typing201 on POST and typed body are consistent with the rest of the client.
1287-1325: update_webhook: PATCH 200 looks rightUses custom-serializing WebhookUpdate; matches SDK patterns.
If Meilisearch returns 202 for async updates in some configurations, confirm 200 is always correct for this endpoint.
1327-1353: delete_webhook: 204 No Content is correctClean and consistent with other delete methods.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #719 +/- ##
==========================================
+ Coverage 85.90% 86.18% +0.28%
==========================================
Files 19 20 +1
Lines 6079 6263 +184
==========================================
+ Hits 5222 5398 +176
- Misses 857 865 +8 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/webhooks.rs (1)
205-253: LGTM! Comprehensive CRUD integration test.The test thoroughly exercises the webhook lifecycle:
- Creates webhook with headers
- Fetches and verifies creation
- Updates headers (remove + add)
- Resets all headers
- Deletes and verifies removal
The test properly handles potential race conditions when verifying deletion (lines 247-250).
Consider adding test coverage for edge cases:
- Creating webhook without headers
- Updating URL without touching headers
- Error handling (e.g., invalid UUID, non-existent webhook)
These would strengthen confidence in error paths and optional field handling.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
.code-samples.meilisearch.yaml(1 hunks)src/webhooks.rs(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/webhooks.rs (2)
src/client.rs (3)
new(54-66)None(1446-1446)None(1525-1525)meilisearch-test-macro/src/lib.rs (1)
meilisearch_test(14-201)
🔇 Additional comments (3)
.code-samples.meilisearch.yaml (1)
1959-1977: LGTM! Webhook code samples look correct.All five webhook samples align well with the new API surface and demonstrate the expected CRUD operations. The samples correctly show:
- Retrieving all webhooks and a single webhook by UUID
- Creating a webhook with custom headers
- Updating a webhook to remove headers
- Deleting a webhook
src/webhooks.rs (2)
144-173: LGTM! Custom serialization correctly implements PATCH semantics.The custom
Serializeimplementation properly handles partial update semantics:
- Only serializes fields that were explicitly set
- Distinguishes between omitted fields (NotSet) and explicit reset (null)
- Correctly serializes header modifications (add/update vs. remove)
This design pattern effectively supports RESTful PATCH operations.
182-203: LGTM! Serialization test correctly validates PATCH semantics.The test effectively verifies:
- Header set/remove operations serialize to the correct JSON structure
- Reset operation serializes headers field to null
- Only modified fields are included in the output
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
bors merge
|
Build succeeded: |
Pull Request
Related issue
Fixes #697
What does this PR do?
Created a new module to support the webhook API.
PR checklist
Please check if your PR fulfills the following requirements:
Thank you so much for contributing to Meilisearch!
Summary by CodeRabbit
New Features
Documentation
Tests