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 try_from attribute for FromRow derive #1081

Merged
merged 1 commit into from
Sep 7, 2022

Conversation

zzhengzhuo
Copy link
Contributor

@zzhengzhuo zzhengzhuo commented Mar 3, 2021

This is used for converting type in FromRow.And I am not sure whether it is worth and just for discussing.

Example

#[derive(sqlx::FromRow)]
struct Record {
    #[sqlx(try_from = "i64")]  
    id: u64,
}

// for custom type
 #[derive(sqlx::FromRow)]
struct Record {
    #[sqlx(try_from = "i64")]
    id: Id,
}

#[derive(Debug, PartialEq)]
struct Id(i64);
impl std::convert::TryFrom<i64> for Id{
    type Error = Error;
    fn try_from(v:i64) -> Result<Id,Self::Error>{
         Ok(Id(v))
    }
}

issues

  • In fact,I open this pr mainly for decoding Bigint to u64,which will be supported in sqlx 6.0+.It makes me confused whether this pr is worth.

There is another attribute with for type converting.Maybe I will open another issue for discussing.

Example for with

 #[derive(sqlx::FromRow)]
struct Record {
    #[sqlx(with(mysql = "path"))]
    id: u64,
}
fn path(value:MySqlValueRef) -> Result<u64,Box<dyn Error + 'static + Send + Sync>>{
  Ok(Decode::<i64>::decode(value)?.try_info()?)
}

@jplatte
Copy link
Contributor

jplatte commented Mar 22, 2021

I don't like try_from as the name for something that takes a function argument. I would expect this to take a type name and use a TryFrom implementation for conversion. I don't really understand the git history here w.r.t. with as an attribute, is that an additional attribute with other semantics that's also being introduced here but not discussed in the PR description?

@zzhengzhuo
Copy link
Contributor Author

zzhengzhuo commented Mar 23, 2021

I don't like try_from as the name for something that takes a function argument. I would expect this to take a type name and use a TryFrom implementation for conversion. I don't really understand the git history here w.r.t. with as an attribute, is that an additional attribute with other semantics that's also being introduced here but not discussed in the PR description?

  1. Maybe it's better to use type name.
  2. In fact,I tried to add with attribute so there is useless git history.
  3. I will edit and add the introduction about with attribute.

@abonander
Copy link
Collaborator

@zzhengzhuo I'm really sorry for leaving this open for so long, when PRs get buried on the second page of results it gets easy to forget about them.

Do you mind rebasing on main to trigger another run of checks so we can see again why the Format job was failing? I see you tried addressing it once before already.

@zzhengzhuo
Copy link
Contributor Author

@abonander Sorry for the delay getting back to you as I am having a busy time. I have rebased on main, please check it.

@zzhengzhuo
Copy link
Contributor Author

But when I run tests, it reports error:

     Running tests/any/any.rs (target/debug/deps/any-3430a2b319f90354)

running 7 tests
test it_connects ... FAILED
test it_does_not_stop_stream_after_decoding_error ... FAILED
test it_gets_by_name ... FAILED
test it_pings ... FAILED
test it_can_fail_and_recover ... FAILED
test it_executes_with_pool ... FAILED
test it_can_fail_and_recover_with_pool ... FAILED

failures:

---- it_connects stdout ----
Error: error communicating with database: received corrupt message

Caused by:
    received corrupt message
thread 'it_connects' panicked at 'assertion failed: `(left == right)`
  left: `1`,
 right: `0`: the test returned a termination value with a non-zero status code (1) which indicates a failure', /rustc/e0944922007e1bb4fe59809293acf4364410cccc/library/test/src/lib.rs:185:5

---- it_does_not_stop_stream_after_decoding_error stdout ----
Error: error communicating with database: received corrupt message

Caused by:
    received corrupt message
thread 'it_does_not_stop_stream_after_decoding_error' panicked at 'assertion failed: `(left == right)`
  left: `1`,
 right: `0`: the test returned a termination value with a non-zero status code (1) which indicates a failure', /rustc/e0944922007e1bb4fe59809293acf4364410cccc/library/test/src/lib.rs:185:5

---- it_gets_by_name stdout ----
Error: error communicating with database: received corrupt message

Caused by:
    received corrupt message
thread 'it_gets_by_name' panicked at 'assertion failed: `(left == right)`
  left: `1`,
 right: `0`: the test returned a termination value with a non-zero status code (1) which indicates a failure', /rustc/e0944922007e1bb4fe59809293acf4364410cccc/library/test/src/lib.rs:185:5

---- it_pings stdout ----
Error: error communicating with database: received corrupt message

Caused by:
    received corrupt message
thread 'it_pings' panicked at 'assertion failed: `(left == right)`
  left: `1`,
 right: `0`: the test returned a termination value with a non-zero status code (1) which indicates a failure', /rustc/e0944922007e1bb4fe59809293acf4364410cccc/library/test/src/lib.rs:185:5

---- it_can_fail_and_recover stdout ----
Error: error communicating with database: received corrupt message

Caused by:
    received corrupt message
thread 'it_can_fail_and_recover' panicked at 'assertion failed: `(left == right)`
  left: `1`,
 right: `0`: the test returned a termination value with a non-zero status code (1) which indicates a failure', /rustc/e0944922007e1bb4fe59809293acf4364410cccc/library/test/src/lib.rs:185:5
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

---- it_executes_with_pool stdout ----
Error: error communicating with database: received corrupt message

Caused by:
    received corrupt message
thread 'it_executes_with_pool' panicked at 'assertion failed: `(left == right)`
  left: `1`,
 right: `0`: the test returned a termination value with a non-zero status code (1) which indicates a failure', /rustc/e0944922007e1bb4fe59809293acf4364410cccc/library/test/src/lib.rs:185:5

---- it_can_fail_and_recover_with_pool stdout ----
[2022-07-10T10:51:59Z ERROR rustls::conn] TLS alert received: AlertMessagePayload {
        level: Fatal,
        description: HandshakeFailure,
    }
Error: error communicating with database: received fatal alert: HandshakeFailure

Caused by:
    received fatal alert: HandshakeFailure
thread 'it_can_fail_and_recover_with_pool' panicked at 'assertion failed: `(left == right)`
  left: `1`,
 right: `0`: the test returned a termination value with a non-zero status code (1) which indicates a failure', /rustc/e0944922007e1bb4fe59809293acf4364410cccc/library/test/src/lib.rs:185:5


failures:
    it_can_fail_and_recover
    it_can_fail_and_recover_with_pool
    it_connects
    it_does_not_stop_stream_after_decoding_error
    it_executes_with_pool
    it_gets_by_name
    it_pings

test result: FAILED. 0 passed; 7 failed; 0 ignored; 0 measured; 0 filtered out; finished in 0.01s

I don't know why.

@mcronce
Copy link
Contributor

mcronce commented Jul 13, 2022

@zzhengzhuo what database are you testing against? I'm seeing identical (passing) results without/with your changes

I also tried rebasing on main again, but the merge conflicts are nontrivial

@abonander
Copy link
Collaborator

@zzhengzhuo sorry for the delay again, the corrupt message error can happen when trying to use RusTLS to connect to an older version of MySQL. It might be a latent bug in SQLx, but I haven't had time to look into it. Try testing with one of the -native-tls runtime features instead, or use a newer version of MySQL to test against.

@zzhengzhuo
Copy link
Contributor Author

@abonander sorry for delay. Please check.

@abonander
Copy link
Collaborator

@zzhengzhuo it's passing but now there's merge conflicts, do you have time to resolve those?

@abonander
Copy link
Collaborator

Great, glad to finally land this!

@abonander abonander merged commit ddffaa7 into launchbadge:main Sep 7, 2022
@mcronce
Copy link
Contributor

mcronce commented Sep 7, 2022

@zzhengzhuo @abonander as a Plain Old User eagerly awaiting this, I appreciate both of you taking the time to get it merged

@arniu
Copy link

arniu commented Sep 9, 2022

try_from => cast?

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.

5 participants