Skip to content
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

add raw json requests docs #196

Merged
merged 3 commits into from
Oct 26, 2023

Conversation

samuelorji
Copy link
Contributor

Description

Add documentation on how to make raw json / http requests using the client

Issues Resolved

#193

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

Signed-off-by: samuel orji <awesomeorji@gmail.com>
@codecov
Copy link

codecov bot commented Oct 24, 2023

Codecov Report

Merging #196 (8f2fb66) into main (13d0d4b) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##             main     #196   +/-   ##
=======================================
  Coverage   73.82%   73.82%           
=======================================
  Files         402      402           
  Lines       63738    63738           
=======================================
  Hits        47055    47055           
  Misses      16683    16683           
Flag Coverage Δ
integration 73.82% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Copy link
Member

@dblock dblock left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's make this a bit more detailed. See opensearch-project/opensearch-py#542 for an example. Add a working sample into samples as well so we can make sure this code runs.

USER_GUIDE.md Outdated
@@ -7,6 +7,7 @@
- [Add a Document to the Index](#add-a-document-to-the-index)
- [Search for a Document](#search-for-a-document)
- [Delete the Index](#delete-the-index)
- [Make raw json requests](#make-raw-json-requests)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Capitalize Titles

USER_GUIDE.md Outdated
@@ -108,6 +109,90 @@ client
.await?;
```

### Make Raw Json Requests

To invoke an API that is not supported by the client, use `client.send` method to do so. see [examples/json](./opensearch/examples/json.rs) for a complete working example
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Make "See ... ." a full sentence with a period.

USER_GUIDE.md Outdated
)
.await?;

assert_eq!(response.status_code().as_u16(), 201_u16);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we really have to cast these in Rust, can't write assert_eq!(response.status_code(), 201)?

USER_GUIDE.md Outdated
)
.await?;

assert_eq!(response.status_code().as_u16(), 200_u16);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this work? Looks like a GET request with a query string, whereas OpenSearch uses POST /.../_search usually.

Look at the response and print some search results?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

According to the docs, both work. The http client we use supports GET requests with a body, either way, i've changed to a POST instead

USER_GUIDE.md Outdated
Option::<&String>::None,
None,
)
.await?;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Check response error code like in the other examples above.

opensearch/examples/json.rs Show resolved Hide resolved
)
.await?;

assert_eq!(response.status_code().as_u16(), 200_u16);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Look at the response and print some search results.

Copy link
Member

@dblock dblock left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like we call the samples directory samples in other clients, not examples.

Signed-off-by: samuel orji <awesomeorji@gmail.com>
@samuelorji
Copy link
Contributor Author

Looks like we call the samples directory samples in other clients, not examples.

I'm not a Rust expert, but I think in Rust, the convention is to put samples in the examples folder

Copy link
Member

@dblock dblock left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, leaving this to @Xtansia .

Copy link
Collaborator

@Xtansia Xtansia left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall looks good, just a couple suggestions to improve the code.

USER_GUIDE.md Outdated
@@ -108,6 +109,94 @@ client
.await?;
```

### Make Raw Json Requests

To invoke an API that is not supported by the client, use `client.send` method to do so. See [examples/json](./opensearch/examples/json.rs) for a complete working example.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: use the client.send method.

Suggested change
To invoke an API that is not supported by the client, use `client.send` method to do so. See [examples/json](./opensearch/examples/json.rs) for a complete working example.
To invoke an API that is not supported by the client, use the `client.send` method to do so. See [examples/json](./opensearch/examples/json.rs) for a complete working example.

USER_GUIDE.md Outdated
Comment on lines 119 to 130
let info: Value = client
.send(
Method::Get,
"/",
HeaderMap::new(),
Option::<&String>::None,
Option::<&String>::None,
None,
)
.await?
.json()
.await?;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When there's no body or query params needed, this can be simplified to just use the unit type () rather than &String. Also specifying the types on send::<> means you don't have to fully qualify Option::<>::None.

Suggested change
let info: Value = client
.send(
Method::Get,
"/",
HeaderMap::new(),
Option::<&String>::None,
Option::<&String>::None,
None,
)
.await?
.json()
.await?;
let info: Value = client
.send::<(), ()>(Method::Get, "/", HeaderMap::new(), None, None, None)
.await?
.json()
.await?;

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes perfect sense, really struggled getting the types to fit 😄

USER_GUIDE.md Outdated
Comment on lines 139 to 156
let index_body = json!({
"settings": {
"index": {
"number_of_shards" : 4
}
}
});

client
.send(
Method::Put,
"/movies",
HeaderMap::new(),
Option::<&String>::None,
Some(index_body.to_string()),
None,
)
.await?;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's better to use the JsonBody type, rather than .to_string, and can work with any type that implements Serialize.

Suggested change
let index_body = json!({
"settings": {
"index": {
"number_of_shards" : 4
}
}
});
client
.send(
Method::Put,
"/movies",
HeaderMap::new(),
Option::<&String>::None,
Some(index_body.to_string()),
None,
)
.await?;
let index_body: JsonBody<_> = json!({
"settings": {
"index": {
"number_of_shards" : 4
}
}
})
.into();
client
.send(
Method::Put,
"/movies",
HeaderMap::new(),
Option::<&()>::None,
Some(index_body),
None,
)
.await?;

USER_GUIDE.md Outdated
Comment on lines 162 to 182
let q = "miller";

let query = json!({
"size": 5,
"query": {
"multi_match": {
"query": q,
"fields": ["title^2", "director"]
}
}
});
client
.send(
Method::Post,
"/movies/_search",
HeaderMap::new(),
Option::<&String>::None,
Some(query.to_string()),
None,
)
.await?;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Similar to above regarding JsonBody

Suggested change
let q = "miller";
let query = json!({
"size": 5,
"query": {
"multi_match": {
"query": q,
"fields": ["title^2", "director"]
}
}
});
client
.send(
Method::Post,
"/movies/_search",
HeaderMap::new(),
Option::<&String>::None,
Some(query.to_string()),
None,
)
.await?;
let q = "miller";
let query: JsonBody<_> = json!({
"size": 5,
"query": {
"multi_match": {
"query": q,
"fields": ["title^2", "director"]
}
}
})
.into();
client
.send(
Method::Post,
"/movies/_search",
HeaderMap::new(),
Option::<&()>::None,
Some(query),
None,
)
.await?;

USER_GUIDE.md Outdated
Comment on lines 188 to 197
client
.send(
Method::Delete,
"/movies",
HeaderMap::new(),
Option::<&String>::None,
Option::<&String>::None,
None,
)
.await?;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Similar to above regarding using unit type ()

Suggested change
client
.send(
Method::Delete,
"/movies",
HeaderMap::new(),
Option::<&String>::None,
Option::<&String>::None,
None,
)
.await?;
client
.send::<(), ()>(
Method::Delete,
"/movies",
HeaderMap::new(),
None,
None,
None,
)
.await?;

&format!("/{index_name}/_doc/{document_id}"),
HeaderMap::new(),
Some(&vec![("refresh", "true")]),
Some(index_body.to_string()),
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should reference the document not the index_body

Suggested change
Some(index_body.to_string()),
Some(document),

)
.await?;

assert_eq!(create_index_response.status_code(), 200);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should reference create_document_response not create_index_response, additionally putting a not yet existing document returns a 201 Created rather than 200

Suggested change
assert_eq!(create_index_response.status_code(), 200);
assert_eq!(create_document_response.status_code(), 201);

Comment on lines 84 to 103
let query = json!({
"size": 5,
"query": {
"multi_match": {
"query": q,
"fields": ["title^2", "director"]
}
}
});

let search_response = client
.send(
Method::Post,
"/movies/_search",
HeaderMap::new(),
Option::<&String>::None,
Some(query.to_string()),
None,
)
.await?;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Following the suggestions about JsonBody and (), also use format! for path to match other requests.

Suggested change
let query = json!({
"size": 5,
"query": {
"multi_match": {
"query": q,
"fields": ["title^2", "director"]
}
}
});
let search_response = client
.send(
Method::Post,
"/movies/_search",
HeaderMap::new(),
Option::<&String>::None,
Some(query.to_string()),
None,
)
.await?;
let query: JsonBody<_> = json!({
"size": 5,
"query": {
"multi_match": {
"query": q,
"fields": ["title^2", "director"]
}
}
})
.into();
let search_response = client
.send(
Method::Post,
&format!("/{index_name}/_search"),
HeaderMap::new(),
Option::<&()>::None,
Some(query),
None,
)
.await?;

Comment on lines 108 to 117
let delete_document_response = client
.send(
Method::Delete,
&format!("/{index_name}/_doc/{document_id}"),
HeaderMap::new(),
Option::<&String>::None,
Option::<&String>::None,
None,
)
.await?;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Following unit-type suggestion

Suggested change
let delete_document_response = client
.send(
Method::Delete,
&format!("/{index_name}/_doc/{document_id}"),
HeaderMap::new(),
Option::<&String>::None,
Option::<&String>::None,
None,
)
.await?;
let delete_document_response = client
.send::<(), ()>(
Method::Delete,
&format!("/{index_name}/_doc/{document_id}"),
HeaderMap::new(),
None,
None,
None,
)
.await?;

Comment on lines 122 to 131
let delete_response = client
.send(
Method::Delete,
&format!("/{index_name}"),
HeaderMap::new(),
Option::<&String>::None,
Option::<&String>::None,
None,
)
.await?;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Following unit-type suggestions:

Suggested change
let delete_response = client
.send(
Method::Delete,
&format!("/{index_name}"),
HeaderMap::new(),
Option::<&String>::None,
Option::<&String>::None,
None,
)
.await?;
let delete_response = client
.send::<(), ()>(
Method::Delete,
&format!("/{index_name}"),
HeaderMap::new(),
None,
None,
None,
)
.await?;

.await?;

assert_eq!(search_response.status_code(), 200);

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can print the results from the search like so:

Suggested change
let search_result = search_response.json::<Value>().await?;
println!("Hits: {:#?}", search_result["hits"]["hits"].as_array().unwrap());

Signed-off-by: samuel orji <awesomeorji@gmail.com>
#### GET
The following example returns the server version information via `GET /`.
```rust
let info: Value = client
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not necessarily the issue, may be using Future combinators (use futures::TryFutureExt;) would be a bit more concise as an example:

Suggested change
let info: Value = client
let info: Value = client.send::<(), ()>(
Method::Get,
"/",
HeaderMap::new(),
None,
None,
None,
)
.and_then(|r| r.json::<Value>())
.await?;

@Xtansia Xtansia merged commit 1041309 into opensearch-project:main Oct 26, 2023
32 checks passed
dblock added a commit to dblock/opensearch-rust-client-demo that referenced this pull request Oct 30, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants