Database borrows builder

[michalmoc]

Or at least lifetimes are resolved so. As result the database cannot be inserted to common data storages: Any requires 'static', so does Anix Extension, likewise SchemaBuilder::data from async-graphql.

[vincent-herlemont]

I did not quite understand the problem you are facing. Could you provide an example and explain in more detail?

[michalmoc]

Trivial example:

    let mut builder = native_db::DatabaseBuilder::new();
    let db = builder.create_in_memory().unwrap();

    let app = Router::new().layer(Extension(Arc::new(db)));

    let listener = tokio::net::TcpListener::bind("0.0.0.0:8000").await.unwrap();
    axum::serve(listener, app).await.unwrap();

It does not compile because

error[E0597]: `builder` does not live long enough
   |
47 |     let mut builder = native_db::DatabaseBuilder::new();
   |         ----------- binding `builder` declared here
48 |     let db = builder.create_in_memory().unwrap();
   |              ^^^^^^^-------------------
   |              |
   |              borrowed value does not live long enough
   |              argument requires that `builder` is borrowed for `'static`
...
83 | }
   | - `builder` dropped here while still borrowed

As far as I know, most frameworks require putting database in some kind of global storage, requiring it to be 'static.

[vincent-herlemont]

@michalmoc I think you can find your answer in these issues:

• https://github.com/vincent-herlemont/native_db/issues/138
• https://github.com/vincent-herlemont/native_db/discussions/69#discussioncomment-7929593
• https://github.com/cberner/redb/issues/558

[michalmoc]

Hmm. These do provide the explanation and work-arounds, but at the same time they serve to prove that something is wrong with this design, since the database is hard to use in common use-cases. Could I hope it will be accepted as an issue to be fixed?

[vincent-herlemont]

Recap: native_db is based on redb, as explained here.

redb has a clear design and requires us to define the tables and then instantiate them, which seems to me a correct pattern, see redb/README.md?plain=1#L17-L23.

In your example, the requirement to put a 'static lifetime on the builder has nothing to do with native_db, but it's the use of axum/tokio that makes your db pass into another thread (see the line let app = Router::new().layer(Extension(Arc::new(db)));, without this there is no need for a 'static lifetime, it's enough that the lifetime 'DatabaseBuilder > 'Database. See for example the sample code in the readme native_db/README.md?plain=1#L116-L121

In your case, the simplest solution is to statically initialize your database as shown here src-tauri/src/main.rs#L45-L54.

Note: If you have a more elegant solution, I’m interested! :)

Maybe I should put a clearer example in the native_db readme so there would be fewer misunderstandings 🤔

[michalmoc]

I woudn't press the issue if it was only one crate, but I believe databases are most commonly used with web frameworks, which tend to be asynchronous and therefore require some 'static storage.

The main problem is with 'Builder'. Builder is quite specific idiom, common in Rust, and introduces some expectations. Usually, when builder is finished, it would move all its content to the new object, and it would be destroyed. So this would be one solution, just move everything from builder to the database struct, and all should work out.

Otherwise, you could drop name 'builder' - with its current functionality, I'd call it DatabaseSchema or something like that. Such name would no longer raise mistaken expectations, although usage in the common use case still won't be obvious. Kind thing to do would be to add an example in some visible spot.

Summa summarum, either:

• move all data from builder to its result
• rename builder and add example usage with 'static in README

[vincent-herlemont]

@michalmoc Thank you for your explanation; you convinced me, you are absolutely right. The idiom of a "builder" type that must stay alive doesn't make sense.

I really like the idea that it could be called DatabaseSchema. However, the current "builder", besides specifying schemas, has this kind of configuration functions set_cache_size (and there will surely be many more in the future) which we typically find as a method of a *Builder type. What is your opinion on this?

move all data from builder to its result

I don't think redb will allow us to do that in the short term. The alternative workarounds involve writing unsafe code, and I don't want to do that.

In any case, you are right; we will need to change that.

[michalmoc]

I don't know the internals. Perhaps those config options can be moved to Databse? Or perhaps a third struct, a real Builder can be crated to handle them? Like, Builder uses Schema to construct Database, and the finished Database references only global Schema.

[vincent-herlemont]

@michalmoc As you advised me, I have externalized the models from the builder. Finally, I simply named it models, which seemed more logical to me given the database design that doesn't have a "schema" per se.

Pr: https://github.com/vincent-herlemont/native_db/pull/164 Example here: https://github.com/vincent-herlemont/native_db/blob/58308fcf46ec9c7110d9a29d50c9781158876b59/README.md#example

Feel free to give me any feedback.

[michalmoc]

@michalmoc As you advised me, I have externalized the models from the builder. Finally, I simply named it models, which seemed more logical to me given the database design that doesn't have a "schema" per se.

Pr: #164 Example here: https://github.com/vincent-herlemont/native_db/blob/58308fcf46ec9c7110d9a29d50c9781158876b59/README.md#example

Feel free to give me any feedback.

Looks good, I've added some minor suggestions in review.

[vincent-herlemont]

@michalmoc Thank you very much for the review!