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

Design and implement the Bakery Chain schema #34

Merged
merged 38 commits into from
Jul 9, 2021
Merged

Design and implement the Bakery Chain schema #34

merged 38 commits into from
Jul 9, 2021

Conversation

samsamai
Copy link
Contributor

@samsamai samsamai commented Jun 26, 2021

This PR implements the example schema for the entities in the Bakery Chain domain and adds basic tests for the creation of these tables.

@samsamai samsamai linked an issue Jun 26, 2021 that may be closed by this pull request
@tyt2y3
Copy link
Member

tyt2y3 commented Jun 26, 2021

Thank you. Seems good so far.

One thing. We can use

db.get_query_builder_backend()

in place of SqliteQueryBuilder to make it backend agnostic.

@tyt2y3
Copy link
Member

tyt2y3 commented Jun 29, 2021

Can you paste the latest ER diagram here again?

@samsamai
Copy link
Contributor Author

bakery_chain_erd

@tyt2y3
Copy link
Member

tyt2y3 commented Jun 29, 2021

One problem. Isn't the many side of the Cake - LineItem relation in the wrong place?
Shouldn't it be line_item.cake_id = cake.id
Such that one cake can appear in many orders?

@samsamai
Copy link
Contributor Author

Yes, I see what you mean. I started looking at Cake as a unique product when I added the 'best before', 'produced at' etc fields.
Cake should probably more general like "Chocolate Cake" and we could have X number of chocolate cakes available. But then what to do with the date/timestamp fields?

@samsamai
Copy link
Contributor Author

Maybe the date/timestamp fields can go on the lineitem

@samsamai samsamai changed the title Design and implement the Bakery Chain domain Design and implement the Bakery Chain schema Jun 30, 2021
@samsamai
Copy link
Contributor Author

bakery_chain_erd

@samsamai samsamai requested a review from tyt2y3 June 30, 2021 11:25
@tyt2y3
Copy link
Member

tyt2y3 commented Jul 1, 2021

We might switch to Decimal on prices once SeaQL/sea-query#67 is done.

tests/bakery_chain/cake.rs Outdated Show resolved Hide resolved
tests/bakery_chain/cake.rs Outdated Show resolved Hide resolved
pub total: f32,
pub bakery_id: Option<i32>,
pub customer_id: Option<i32>,
pub placed_at: String,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please use sea_query's NaiveDateTime support on timestamp.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@tyt2y3 when I use NaiveDateTime from chrono
pub placed_at: NaiveDateTime,

I get the following errors about the trait bounds not satisfied:

   Compiling sea-orm v0.1.0 (/Users/sam/Code/github/sea-orm)
error[E0277]: the trait bound `NaiveDateTime: std::default::Default` is not satisfied
  --> tests/bakery_chain/order.rs:20:48
   |
20 | #[derive(Clone, Debug, PartialEq, DeriveModel, DeriveActiveModel)]
   |                                                ^^^^^^^^^^^^^^^^^ the trait `std::default::Default` is not implemented for `NaiveDateTime`
   |
  ::: /Users/sam/Code/github/sea-orm/src/entity/active_model.rs:10:22
   |
10 |     V: Into<Value> + Default,
   |                      ------- required by this bound in `ActiveValue`
   |
   = note: this error originates in a derive macro (in Nightly builds, run with -Z macro-backtrace for more info)

error: aborting due to previous error

For more information about this error, try `rustc --explain E0277`.
error: could not compile `sea-orm`

To learn more, run the command again with --verbose.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

732428f
I removed the Default bound from the trait ActiveValue
Should be okay now?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@tyt2y3 it's getting further now but there are still some issues. Please see the this branch: https://github.com/SeaQL/sea-orm/tree/nativedatetime

and test using:

cargo test --test bakery_chain_tests -- --nocapture

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Still need help on this? Or you can learn from what I did on Decimal support?

@tyt2y3
Copy link
Member

tyt2y3 commented Jul 4, 2021

Thank you. Looks fine so far. Please continue. But I also want to use more data types of 'sea_query::Value' so as to make the coverage more complete.

@tyt2y3
Copy link
Member

tyt2y3 commented Jul 5, 2021

Nice. Have you tried running it against a MySQL instance?

@tyt2y3
Copy link
Member

tyt2y3 commented Jul 6, 2021

SeaQL/sea-query#67
Decimal support added

@samsamai
Copy link
Contributor Author

samsamai commented Jul 7, 2021

SeaQL/sea-query#67
Decimal support added

With regards to using Decimal for our prices, would our model look something like this:

#[derive(Clone, Debug, PartialEq, DeriveModel, DeriveActiveModel)]
pub struct Model {
    pub id: i32,
    pub price: Decimal,
    pub quantity: i32,
    pub order_id: Option<i32>,
    pub cake_id: Option<i32>,
}

If so where is Decimal coming from? Directly from rust_decimal or through SeaQuery?
Coming directly from rust_decimal I ran into some issues with traits:

error[E0277]: the trait bound `sea_orm::Value: From<rust_decimal::Decimal>` is not satisfied
  --> tests/bakery_chain/lineitem.rs:13:48
   |
13 | #[derive(Clone, Debug, PartialEq, DeriveModel, DeriveActiveModel)]
   |                                                ^^^^^^^^^^^^^^^^^ the trait `From<rust_decimal::Decimal>` is not implemented for `sea_orm::Value`
   |
  ::: /Users/sam/Code/github/sea-orm/src/entity/active_model.rs:10:8
   |
10 |     V: Into<Value>,
   |        ----------- required by this bound in `ActiveValue`
   |

Or do we just use f64 in the model?

@tyt2y3
Copy link
Member

tyt2y3 commented Jul 7, 2021

Sorry I forgot to impl that trait.
The Decimal should come from rust_decimal.
Now added SeaQL/sea-query@dcd27d4#diff-be1c878c753064b7eaddf4a10ba4215a93dd1817d31e6788691cdb29752fd612R620
Note: you need to upgrade your local sea-query to 0.12.5

This was referenced Jul 7, 2021
@tyt2y3
Copy link
Member

tyt2y3 commented Jul 7, 2021

I have fixed the build errors.
But then I discovered that sqlx with SQLite does not support deserializing to Decimal
So we can only run the test suite on MySQL & Postgres.
Edit: I tried to support Decimal on SQLite

@tyt2y3
Copy link
Member

tyt2y3 commented Jul 8, 2021

Nice. I am glad we made it work now!

@samsamai
Copy link
Contributor Author

samsamai commented Jul 9, 2021

Nice. I am glad we made it work now!

Thank you!

@tyt2y3
Copy link
Member

tyt2y3 commented Jul 9, 2021

How about NaiveDateTime, are you also trying?

@samsamai
Copy link
Contributor Author

samsamai commented Jul 9, 2021

How about NaiveDateTime, are you also trying?

@tyt2y3 I was getting this:

   Compiling sea-orm v0.1.0 (/Users/sam/Code/github/sea-orm)
error[E0277]: the trait bound `NaiveDateTime: std::default::Default` is not satisfied
   --> tests/bakery_chain/order.rs:14:48
    |
14  | #[derive(Clone, Debug, PartialEq, DeriveModel, DeriveActiveModel)]
    |                                                ^^^^^^^^^^^^^^^^^ the trait `std::default::Default` is not implemented for `NaiveDateTime`
    |
   ::: /Users/sam/.rustup/toolchains/stable-x86_64-apple-darwin/lib/rustlib/src/rust/library/core/src/mem/mod.rs:761:16
    |
761 | pub fn take<T: Default>(dest: &mut T) -> T {
    |                ------- required by this bound in `std::mem::take`
    |
    = note: required because of the requirements on the impl of `std::default::Default` for `ActiveValue<NaiveDateTime>`
    = note: this error originates in a derive macro (in Nightly builds, run with -Z macro-backtrace for more info)

error: aborting due to previous error

For more information about this error, try `rustc --explain E0277`.
error: could not compile `sea-orm`

To learn more, run the command again with --verbose.

So I've made placed_at: pub placed_at: Option<NaiveDateTime>,

@tyt2y3
Copy link
Member

tyt2y3 commented Jul 9, 2021

okay so, I think the problem is you merged wrongly.
It's getting hard to untangle now.
Let me merge it first.

@tyt2y3 tyt2y3 merged commit 883bd16 into master Jul 9, 2021
@tyt2y3
Copy link
Member

tyt2y3 commented Jul 9, 2021

It should be fine if we branch off again from master and add in NaiveDateTime now.
Thank you for now.

SebastienGllmt pushed a commit to dcSpark/sea-orm that referenced this pull request Apr 13, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Bakery Schema Design
2 participants