Astrazione del body della request #9

matteosister · 2020-10-20T12:47:41Z

Non richiede più un Serializable ma chiede qualsiasi cosa convertibile in un Body. Apre la possibilità di usare un binary come body in una request REST.

Ad oggi ho solo deprecato la vecchia parte. Prossima release butto via tutto...

Nella parte dei test si può vedere cosa cambia nell'utilizzo dell'api.
In pratica da

let body: Option<String> = None;
bridge.request(RequestType::rest(body, Method::GET)).send();

diventa

RestRequest::new(&bridge).send();

(GET è di default, così come il body vuoto)

Tutto è nato dall'esigenza di usare binary body con pasteur, ma in generale preferisco la nuova api. Mi sembra molto più tersa. Voi che dite?

Qui si vede come passare il body alla request in costruzione.

Una cosa che non mi piace molto è la possibilità di usare raw_body con una request graphql, che non ha molto senso. Se dovesse diventare un problema si potrebbero dividere completamente i due tipi RestRequest e GraphQLRequest, eliminando il trait DeliverableRequest. Ma questo vorrebbe dire un sacco di codice duplicato, quindi preferisco per ora mantenerlo così...

Guara92

Ripensando un attimo alla struttura delle request pensavo che forse la request dovrebbe essere indipendente dal bridge essendo solo il "mezzo" su cui viene eseguita, l'api potrebbe essere qualcosa tipo bridge.send(RestRequest::new()) o RestRequest::new().send(&bridge)

src/request.rs

src/v2/body.rs

Guara92 · 2020-10-21T09:28:43Z

src/v2/body.rs

+use crate::errors::PrimaBridgeError;
+use std::convert::TryFrom;
+
+#[derive(Clone, Debug)]


perché il body di una request dovrebbe essere clonabile? forse dovrebbe contenere solo un riferimento all'array di bytes

perchè credo che reqwest sotto si prenda l'ownership del body, quindi non posso passarlo per referenza....

prenderei in considerazione di consumare la DeliverableRequest implementando

impl<T: DeliverableRequest> From<T> for RequestBuilder

dove consumi la "nostra" request e ottieni un builder di reqwest in un colpo solo evitando diversi clone

interessante....da esplorare...

è un po' un casino perchè nel trait ora ci sono le due implementazioni di send per le due feature async e blocking. Il RequestBuilder è diverso a sua volta in base ad async e blocking...quindi non hanno un interfaccia comune

chess4ever

sono ignorante in rust, quindi il massimo contributo che posso dare è provare a integrarlo su toretto con la chiamata a pasteur e farci un qa

matteosister · 2020-10-21T15:38:29Z

@chess4ever che è tantissimo!

@Guara92 bridge.send non è per niente male...ma anche RestRequest::new(&bridge) non mi dispiace.

chess4ever · 2020-10-21T15:39:28Z

@chess4ever che è tantissimo!

@Guara92 bridge.send non è per niente male...ma anche RestRequest::new(&bridge) non mi dispiace.

Aspetto un tuo ok e poi volo!

miterst · 2020-10-22T08:33:04Z

Stavo pensando forse si puo' fare anche piu' breve es.:

Request::get(&bridge).send() # ::get(..) puo ritornare RestRequest::new(&bridge)
Request::post(&bridge).body(..).send() 
.
.
Request::graphql(&bridge).body(..).send() # ::graphql(..) puo ritornare GraphqlRequest::new(&bridge)

Che ne pensate?

matteosister · 2020-10-22T09:10:25Z

Stavo pensando forse si puo' fare anche piu' breve es.:

Request::get(&bridge).send() # ::get(..) puo ritornare RestRequest::new(&bridge)
Request::post(&bridge).body(..).send() 
.
.
Request::graphql(&bridge).body(..).send() # ::graphql(..) puo ritornare GraphqlRequest::new(&bridge)

Che ne pensate?

molto bello!

matteosister · 2020-10-22T12:07:36Z

Ho implementato le cose che diceva @Guara92 (non sulla parte request che ora è deprecata e a breve si butta) e l'api fancy suggerita da @miterst.

torrefatto

Bel lavoro di refactoring. Ho solo il dubbio che non stiamo testando veramente l'invio di un payload non utf8-serializalbe.

torrefatto · 2020-10-22T13:11:50Z

src/prelude.rs

@@ -1,6 +1,6 @@
 pub use super::errors::*;
 pub use super::{
-    request::{GraphQL, RequestType, Rest},


Così però chi usa la versione precedente importando dal prelude si rompe...

Vero! Ma siamo in versione 0.*, così forziamo l'update...

torrefatto · 2020-10-22T14:15:37Z

tests/blocking/rest.rs

+#[test]
+fn request_post_with_custom_raw_body() -> Result<(), Box<dyn Error>> {
+    let body = "abcde";
+    let _mock = mock("POST", "/")
+        .match_body(Matcher::Exact(body.to_owned()))
+        .with_status(200)
+        .create();
+
+    let url = Url::parse(mockito::server_url().as_str()).unwrap();
+    let bridge = Bridge::new(url);
+
+    let result = Request::post(&bridge).raw_body(body).send();
+    assert!(result.is_ok());
+    Ok(())
+}


Questo testa sempre l'invio di una stringa, giusto? Non ho capito benissimo se raw_body accetta Vec<u8>.

eh ma c'è il vecchio tema.. con mockito non puoi "ancora" fare il match dei binary

la funzione .raw_body() accetta un impl ToString. Questo vuol dire che effettivamente ad ora non puoi passare direttamente un Vec, ma dovresti fare così:

let binary:Vec<u8> = "pippo".to_string().into_bytes(); let body = std::str::from_utf8(&binary)?; let result = Request::post(&bridge).raw_body(body).send();

Quindi mi aspetto che chi passa il raw_body si accerti che sia una lista valida di caratteri utf-8. La gestione di questa conversione non credo debba stare nella libreria perchè è forse out of scope.

Tra l'altro esiste anche una versione che non ritorna result from_utf8_unchecked.

In from_utf8 c'è la spiegazione del perchè eistono le due versioni (utile solo a me perchè sono ignorante)

@torrefatto e altri che ne dite? Secondo voi devo fare la conversione all'interno della libreria e accettare un tipo ancora più generico?

non mi torna.. pensavo fosse possibile ora inviare direttamente un Vec<u8>; lo use case di Toretto per le chiamate verso Pasteur è proprio questo, inviare un binary che non è utf-8

no ora ci provo..

a proposito @torrefatto e @chess4ever siete ancora in tempo per provare a integrarlo?
Potreste fare una prova con questo branch?

perchè non mi sento proprio 100% confident

sai come fare? Penso che tu possa usarlo come submodule...o magari cargo ti lascia referenziare un branch?

si si può! https://doc.rust-lang.org/cargo/reference/specifying-dependencies.html#specifying-dependencies-from-git-repositories 😎

wolf4ood · 2020-10-23T07:48:23Z

Molto belle le nuove API :)

Sarebbe fico come accennavi tu, rendere impossibile una cosa del genere

// ok
Request::post(&bridge).raw_body("abcde").send().await;

// error
Request::graphql(&bridge, (query, variables))?.raw_body("abcde").send().await

anche su altri metodi tipo to method ad esempio

matteosister · 2020-10-23T09:43:31Z

Molto belle le nuove API :)

Sarebbe fico come accennavi tu, rendere impossibile una cosa del genere
// ok
Request::post(&bridge).raw_body("abcde").send().await;

// error
Request::graphql(&bridge, (query, variables))?.raw_body("abcde").send().await
anche su altri metodi tipo to method ad esempio

@wolf4ood ❤️ per ora ho implementato qualcosina...https://github.com/primait/bridge.rs/blob/v2/src/v2/request/mod.rs

matteosister · 2020-10-23T09:45:25Z

@wolf4ood secondo te facciamo proprio tipi separati? Tipo typestate pattern, oppure faccio tornare un result dalle funzioni incriminate?

wolf4ood · 2020-10-23T12:11:30Z

Si tipo una cosa del genere

struct GraphQL;

struct Rest;


struct Request<T> {
   
}

impl Request<Rest> {
    
    fn json_body(self) -> Self {
        self
    }
}

impl<T> Request<T> {
    fn send(self) {
        
    } 
}
....
}

mettendo sul Rest quelli che hanno poco senso sul GraphQL

Guara92 · 2020-10-23T13:46:09Z

src/v2/mod.rs

@@ -43,7 +43,7 @@ pub trait DeliverableRequest<'a>: Sized + 'a {
    /// adds a new set of headers to the request. Any header already present gets removed.
    fn set_custom_headers(self, headers: Vec<(HeaderName, HeaderValue)>) -> Self;

-    /// add a custom header to the set of request headers
+    /// add a custom header to the set of request headersbeh 500


credo ti sia scappato

matteosister added 4 commits October 15, 2020 17:43

new approach with distinct requests

69fd432

seems to work

a135778

tests ok

0069d75

adds deprecations and finalize tests

413f9a4

matteosister linked an issue Oct 20, 2020 that may be closed by this pull request

Rendere possibile l'invio di un body in formato binario #7

Closed

matteosister added 6 commits October 20, 2020 17:19

docs

714327e

re-adds the GraphQLBody into trait

93ae9bd

removes dbg! calls

c269dea

removes multiple get_url calls, and pub fields of structs

17d17ab

removes unused dep

23f8008

test for json body

95fe821

matteosister requested review from wolf4ood, chess4ever, Guara92, miterst and torrefatto October 21, 2020 08:32

matteosister marked this pull request as ready for review October 21, 2020 08:32

Guara92 reviewed Oct 21, 2020

View reviewed changes

chess4ever reviewed Oct 21, 2020

View reviewed changes

matteosister added 2 commits October 22, 2020 14:03

review @mitrest e @Guara92

078c147

adds rest request

5b0f5c6

matteosister requested a review from Guara92 October 22, 2020 12:07

better readme with code example

65f7f84

torrefatto reviewed Oct 22, 2020

View reviewed changes

Into<Body> for Vec<u8>

919e2e5

matteosister added 2 commits October 22, 2020 18:06

impl esplicite

34cc002

clippy

387ad61

content-type in all json requests

aafd4b7

matteosister added 3 commits October 23, 2020 14:27

with_custom_headers do not ovverride anymore

412ee6c

some docs

256b0b3

adds Request to prelude

28165c9

Guara92 reviewed Oct 23, 2020

View reviewed changes

chess4ever previously approved these changes Oct 23, 2020

View reviewed changes

review

cad6489

matteosister dismissed chess4ever’s stale review via cad6489 October 26, 2020 10:13

review

833c173

matteosister merged commit c20096f into master Oct 26, 2020

matteosister deleted the v2 branch October 26, 2020 10:53

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Astrazione del body della request #9

Astrazione del body della request #9

matteosister commented Oct 20, 2020 •

edited

Loading

Guara92 left a comment

Guara92 Oct 21, 2020

matteosister Oct 21, 2020

Guara92 Oct 22, 2020 •

edited

Loading

matteosister Oct 22, 2020

matteosister Oct 22, 2020

chess4ever left a comment

matteosister commented Oct 21, 2020

chess4ever commented Oct 21, 2020

miterst commented Oct 22, 2020

matteosister commented Oct 22, 2020

matteosister commented Oct 22, 2020

torrefatto left a comment

torrefatto Oct 22, 2020

matteosister Oct 22, 2020

torrefatto Oct 22, 2020

chess4ever Oct 22, 2020

matteosister Oct 22, 2020 •

edited

Loading

chess4ever Oct 22, 2020 •

edited

Loading

chess4ever Oct 22, 2020

matteosister Oct 22, 2020

matteosister Oct 22, 2020

chess4ever Oct 22, 2020

matteosister Oct 22, 2020

matteosister Oct 22, 2020

wolf4ood commented Oct 23, 2020

matteosister commented Oct 23, 2020 •

edited

Loading

matteosister commented Oct 23, 2020

wolf4ood commented Oct 23, 2020 •

edited

Loading

Guara92 Oct 23, 2020

Astrazione del body della request #9

Astrazione del body della request #9

Conversation

matteosister commented Oct 20, 2020 • edited Loading

Guara92 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Guara92 Oct 22, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chess4ever left a comment

Choose a reason for hiding this comment

matteosister commented Oct 21, 2020

chess4ever commented Oct 21, 2020

miterst commented Oct 22, 2020

matteosister commented Oct 22, 2020

matteosister commented Oct 22, 2020

torrefatto left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

matteosister Oct 22, 2020 • edited Loading

Choose a reason for hiding this comment

chess4ever Oct 22, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wolf4ood commented Oct 23, 2020

matteosister commented Oct 23, 2020 • edited Loading

matteosister commented Oct 23, 2020

wolf4ood commented Oct 23, 2020 • edited Loading

Choose a reason for hiding this comment

matteosister commented Oct 20, 2020 •

edited

Loading

Guara92 Oct 22, 2020 •

edited

Loading

matteosister Oct 22, 2020 •

edited

Loading

chess4ever Oct 22, 2020 •

edited

Loading

matteosister commented Oct 23, 2020 •

edited

Loading

wolf4ood commented Oct 23, 2020 •

edited

Loading