Initial Version #2

jrconlin · 2018-04-07T00:07:22Z

This is a rust implementation of PushBox and is a work in progress.

bbangert · 2018-04-09T19:54:09Z

src/db/models.rs

+                    .filter(pushboxv1::device_id.eq(device_id.clone())),
+            ).execute(conn)
+                .context(HandlerErrorKind::DBError)
+                .unwrap();


This will never fail?

It may, but IIUC, the context() function captures the error and handles it as a HandleErrorKind::DBError https://boats.gitlab.io/failure/doc/failure/trait.Fail.html#method.context Although, I admit that it's through the failure crate, so there's some voodoo going on there.

Right you'll convert the diesel Error into a failure here but you still ultimately end up with a Result that you don't want to unwrap() (panic).

This function already returns a HandlerError so instead propagate it out via try!: .context(HandlerErrorKind::DBError)?;

bbangert · 2018-04-09T19:54:17Z

src/db/models.rs

+                    .filter(pushboxv1::service.eq(service.clone())),
+            ).execute(conn)
+                .context(HandlerErrorKind::DBError)
+                .unwrap();


Bare unwrap?

Same as above. The context should provide additional handling information.

pjenvey · 2018-04-09T23:59:16Z

src/server.rs

+                .unwrap_or(-1);
+            if decoded > -1 {
+                match key.to_lowercase().as_str() {
+                    "index" => opt.index = Some(decoded.clone()),


nitpick: don't need to call .clone() on these primitives here (they are Copy types)

pjenvey · 2018-04-09T23:59:43Z

src/server.rs

+
+#[derive(Debug)]
+pub struct Options {
+    pub index: Option<i64>,


nitpick: these and default_ttl could really be u64s

yeah, except that diesel maps BigInt to i64 http://docs.diesel.rs/src/diesel/sql_types/mod.rs#102, and changing to u64 triggers a compiler issue:

the trait `diesel::Expression` is not implemented for `u64`

so i64 it is.

also: diesel-rs/diesel#80 (comment)

meh, going to hack around it. Not using u64s bugs me too.

pjenvey · 2018-04-10T00:57:26Z

src/server.rs

+    }
+    Ok(Json(json!({
+        "last": is_last,
+        "index": max_index.clone(),


same as above

pjenvey · 2018-04-10T01:10:00Z

src/server.rs

+    let messages = match DatabaseManager::read_records(&conn, &user_id, &device_id, &service, 0, 0)
+    {
+        Ok(val) => val,
+        Err(e) => return Err(e),


this match statement and the one in DatabaseManager::new_record should be killed for try! (let messages = ...)?;)

pjenvey · 2018-04-10T01:18:52Z

src/db/models.rs

+            .expect("Could not create transaction");
+        insert_into(pushboxv1::table)
+            .values((
+                pushboxv1::user_id.eq(user_id.to_string()),


none of the to_string or cloned calls are needed for these diesel queries, it's ok w/ &strs

pjenvey · 2018-04-10T16:10:50Z

src/server.rs

+        &data.data,
+        calc_ttl(data.ttl),
+    );
+    if response.is_err() {


same here and in delete, you can replace these 3 lines by using the ? operator on the previous statement

pjenvey · 2018-04-10T16:13:06Z

src/auth.rs

+            // There is no other way to get the rocket.config() from inside a request
+            // handler.
+            let config = request.guard::<State<RustboxConfig>>().unwrap();
+            let fxa_host = config.fxa_host.clone();


this could be referenced (= &config.fxa_host) vs clone()'d

pjenvey · 2018-04-10T16:59:48Z

src/server.rs

+        &data.data,
+        calc_ttl(data.ttl),
+    );
+    if response.is_err() {


same w/ these, just use ? on previous statement

pjenvey · 2018-04-10T17:00:12Z

src/auth.rs

+                            ))
+                        }
+                    };
+                    if raw_resp.status().is_success() == false {


!raw_resp.status().is_success()

pjenvey · 2018-04-10T17:03:08Z

src/auth.rs

+    fn test_valid() {
+        let mut test_data = Table::new();
+        let mut fxa_response = Table::new();
+        fxa_response.insert("user".to_owned(), "test".to_owned().into());


you can replace .to_owned.into() with just .into() for all these values

pjenvey · 2018-04-10T21:21:42Z

src/server.rs

+    }
+    Ok(Json(json!({
+        "last": is_last,
+        "index": max_index.clone(),


isn't "index" returned here the last index in the current batch we are returning to the client? So it knows where it left off. max_index here is the last index of all messages, which isn't right.

pjenvey · 2018-04-10T22:38:26Z

src/db/models.rs

+        service: &String,
+    ) -> HandlerResult<bool> {
+        // boxed deletes are "coming soon"
+        // see https://github.com/diesel-rs/diesel/pull/1534


they're here! 1.2.0 was just released w/ them

bbangert · 2018-04-25T00:56:12Z

The commits should probably be squashed at this point, and verified.

bbangert · 2018-04-25T16:37:11Z

No .travis.yml for a basic build test?

pjenvey

this looks about ready to merge to mozilla-services/pushbox master (minus circleci and a deployment setup) let alone rustbox master

pjenvey · 2018-04-25T16:39:19Z

src/auth/mod.rs

+        token: String,
+        config: &ServerConfig,
+        logger: &RBLogger,
+    ) -> request::Outcome<Self, HandlerError> {


now that this is separate from a request guard, it can return Result instead of Outcome (which can let the error handling code become easier)

I'm not so sure. Both of these are used by from_request which wants an Outcome response. I'm sure there's some cleanup that could be done there, but having both of these return Outcome I think still makes sense.

pjenvey · 2018-04-25T16:42:43Z

src/db/models.rs

+        let t_manager = conn.transaction_manager();
+        t_manager
+            .begin_transaction(conn)
+            .expect("Could not create transaction");


error should never happen, but maybe there's a once in a million db/network hiccup: safer to use .context(HandlerErrorKind::DBError)? instead

pjenvey · 2018-04-25T16:42:52Z

src/db/models.rs

+        };
+        t_manager
+            .commit_transaction(conn)
+            .expect("Could not close transaction");


same as begin_transaction

This is a rust implementation of PushBox and is a work in progress.

jrconlin · 2018-04-25T20:14:16Z

No, travis will be coming shortly, once I iron out the docker stuff.

jrconlin requested review from pjenvey and bbangert April 7, 2018 00:08

bbangert reviewed Apr 9, 2018

View reviewed changes

jrconlin force-pushed the wip branch 2 times, most recently from 2fb1ab2 to 038cc33 Compare April 10, 2018 00:14

pjenvey suggested changes Apr 10, 2018

View reviewed changes

pjenvey reviewed Apr 10, 2018

View reviewed changes

jrconlin force-pushed the wip branch 2 times, most recently from 6c36751 to 85ed73c Compare April 19, 2018 17:50

bbangert approved these changes Apr 25, 2018

View reviewed changes

pjenvey approved these changes Apr 25, 2018

View reviewed changes

jrconlin force-pushed the wip branch 2 times, most recently from 62c955f to cc85aab Compare April 25, 2018 19:37

Initial Version

9e9693e

This is a rust implementation of PushBox and is a work in progress.

jrconlin force-pushed the wip branch from cc85aab to 9e9693e Compare April 25, 2018 20:11

jrconlin merged commit bfe5a4b into master Apr 25, 2018

bbangert deleted the wip branch April 25, 2018 21:33

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Initial Version #2

Initial Version #2

jrconlin commented Apr 7, 2018

bbangert Apr 9, 2018

jrconlin Apr 9, 2018

pjenvey Apr 9, 2018 •

edited

bbangert Apr 9, 2018

jrconlin Apr 9, 2018

pjenvey Apr 9, 2018

pjenvey Apr 9, 2018

jrconlin Apr 11, 2018

jrconlin Apr 11, 2018

jrconlin Apr 11, 2018

pjenvey Apr 10, 2018

pjenvey Apr 10, 2018

pjenvey Apr 10, 2018

pjenvey Apr 10, 2018

pjenvey Apr 10, 2018

pjenvey Apr 10, 2018

pjenvey Apr 10, 2018

pjenvey Apr 10, 2018

pjenvey Apr 10, 2018

pjenvey Apr 10, 2018

bbangert commented Apr 25, 2018

bbangert commented Apr 25, 2018

pjenvey left a comment

pjenvey Apr 25, 2018 •

edited

jrconlin Apr 25, 2018

pjenvey Apr 25, 2018

pjenvey Apr 25, 2018

jrconlin commented Apr 25, 2018

Initial Version #2

Initial Version #2

Conversation

jrconlin commented Apr 7, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pjenvey Apr 9, 2018 • edited

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

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

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

bbangert commented Apr 25, 2018

bbangert commented Apr 25, 2018

pjenvey left a comment

Choose a reason for hiding this comment

pjenvey Apr 25, 2018 • edited

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

jrconlin commented Apr 25, 2018

pjenvey Apr 9, 2018 •

edited

pjenvey Apr 25, 2018 •

edited