kurtbuilds / ormlite

An ORM in Rust for developers that love SQL.
https://crates.io/crates/ormlite
MIT License
216 stars 11 forks source link

0.3.2 cannot use primary keys that are non Copy #13

Closed AndrewRademacher closed 1 year ago

AndrewRademacher commented 2 years ago
test tests/01-update-partial.rs ... error
┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈
error[E0507]: cannot move out of a shared reference
 --> tests/01-update-partial.rs:8:10
  |
8 | #[derive(Model, FromRow)]
  |          ^^^^^
  |          |
  |          move occurs because value has type `String`, which does not implement the `Copy` trait
  |          help: consider borrowing here: `&Model`
  |
  = note: this error originates in the derive macro `Model` (in Nightly builds, run with -Z macro-backtrace for more info)
┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈

test tests ... FAILED

failures:

---- tests stdout ----
thread 'tests' panicked at '1 of 1 tests failed', /Users/andrew/.cargo/registry/src/github.com-1ecc6299db9ec823/trybuild-1.0.63/src/run.rs:101:13
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

failures:
    tests

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

error: test failed, to rerun pass '--test tests'
error: Recipe `test` failed on line 28 with exit code 101
error: Recipe `test` failed on line 2 with exit code 101

After modifying the test example to be:

#[derive(Model, FromRow)]
pub struct Person {
    id: Uuid,
    #[ormlite(primary_key)]
    name: String,
    age: u8,
}

I'm hunting for the issue, but I'm fairly new to defining macros.

kurtbuilds commented 2 years ago

Yeah, this makes sense. I can test whether there's a straight-forward fix.

What's the use case for a non-copy pkey though? In the wild, I've only seen integers and uuids.

AndrewRademacher commented 2 years ago

In my case the key was just a String, that I use to store a Url.

kurtbuilds commented 2 years ago

I poked at this issue. I'm sure it's fixable, but it's apparently nontrivial because the macro has to behave differently depending on whether the type of the primary key field implements Copy or not.

If this is an important bug, I can prioritize fixing it - but right now don't see a critical need.

0xdeafbeef commented 1 year ago

I poked at this issue. I'm sure it's fixable, but it's apparently nontrivial because the macro has to behave differently depending on whether the type of the primary key field implements Copy or not.

If this is an important bug, I can prioritize fixing it - but right now don't see a critical need.

We have a lot of composite keys, consisting of several strings or Vec<u8> | [u8; 32]

jeremyarde commented 1 year ago

I am running into this issue now - I generate nanoid keys on the server and want to use those as my IDs instead of Uuid or an autoincrementing integer.

Its not a deal breaker, I'll just create a new field on the struct for my nanoid but figured this is another datapoint for you to gauge priority.

teenjuna commented 1 year ago

+1. Tried to implement SessionStore, but couldn't since by design the id is String.

it's apparently nontrivial because the macro has to behave differently depending on whether the type of the primary key field implements Copy or not.

@kurtbuilds can you just assume that type implements Clone? Every Copy type implements Clone, so that will expand the spectre of possible types while not decreasing performance (Clone of Copy types usually just uses copy under the hood?). It shouldn't even be a breaking change.

kurtbuilds commented 1 year ago

Yeah that's a great point. Relaxing to Clone does work here. PR welcome, otherwise I'll update this when I can get to this.

kurtbuilds commented 1 year ago

This should be fixed in 0.10.0. Please let me know if you encounter any issues.