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

BREAKING CHANGES: Update database schema #111

Closed
2 tasks
chrisjm opened this issue Jan 3, 2022 · 7 comments · Fixed by #145
Closed
2 tasks

BREAKING CHANGES: Update database schema #111

chrisjm opened this issue Jan 3, 2022 · 7 comments · Fixed by #145

Comments

@chrisjm
Copy link
Contributor

chrisjm commented Jan 3, 2022

Tasks

  • Rename obdb_id to slug
  • Update id to use UUID

Notes

  • This should be done after the versioning is migrated (it might already have)
  • The API will also need to be updated
@alexjustesen
Copy link
Contributor

alexjustesen commented Jan 3, 2022

Based on our conversation below is a model of the current breweries table along with the proposed changes. This model currently implements a "Type 2" format where the current record has no end_at date.

Model: https://dbdiagram.io/d/61d360353205b45b73d55971

Considerations

  • For start_at and end_at do we really care about the time or is the date good enough?

Todo

  • Column settings like restrictions on length, nullable and any FKs should be documented
  • There should probably be constraint aka 2 part key for the field tbd and end_at to prevent duplicates
  • Type 2 has no record identifier right now tying the history of a model

Changelog

Added

  • start_at and end_at to make the table a type 2 for history

Changed

  • obdb_id uses uuid v4 instead of a slug from the name
  • brewery_type to type, context is already set as the brewery no need to duplicate it
  • street changed to address_line_1 for consistency
  • address_2 and address_3 renamed to address_line_x
  • country_province combines the state and country_province columns to support foreign addresses
  • country limited to 2 characters to support only the ISO code format https://www.countrycode.org/
  • postal_code shortened to postcode

@chrisjm chrisjm changed the title Use UUID for id; change obdb_id to slug BREAKING CHANGES: Update database schema Jan 4, 2022
@chrisjm
Copy link
Contributor Author

chrisjm commented Jan 4, 2022

@alexjustesen This is more of what I'm thinking about with the schema (I also updated the v1 to be more based on reality): https://dbdiagram.io/d/61d3c9533205b45b73d5a738

Table breweries_v1 {
  id integer [pk]
  obdb_id varchar
  name varchar
  brewery_type varchar
  street varchar
  address_2 varchar
  address_3 varchar
  city varchar
  state varchar
  postal_code varchar
  county_province varchar
  country varchar
  longitude decimal
  latitude decimal
  website_url varchar
  phone varchar
  tags text
  created_at datetime
  updated_at datetime
}

Table breweries_v2 as proposed {
  id uuid [pk]
  name varchar
  brewery_type varchar
  address_line_1 varchar
  address_line_2 varchar
  address_line_3 varchar
  city varchar
  state_province varchar
  postcode varchar
  country varchar
  longitude decimal
  latitude decimal
  website_url varchar
  phone varchar
  created_at datetime
  updated_at datetime
  start_at date
  end_at date
 }

We still have the "key problem" you mention in your third TODO. I guess just another UUID? Would be nice to have it be shorter. Maybe something based on lat/lng or would that be dumb?

@chrisjm
Copy link
Contributor Author

chrisjm commented Jan 4, 2022

Oh, and after thinking about it, the whole "slug" thing should be handled programmatically or in a mapping table. Seems a little silly to hard-code it here.

@alexjustesen
Copy link
Contributor

Oh, and after thinking about it, the whole "slug" thing should be handled programmatically or in a mapping table. Seems a little silly to hard-code it here.

I agree, the slug for SEO purposes should be generated off the name where it's being used.

@alexjustesen
Copy link
Contributor

I removed slug from my model and changed brewery_type to just type as the context is already the brewery so no need to duplicate the name.

@chrisjm
Copy link
Contributor Author

chrisjm commented Jan 7, 2022

@alexjustesen Sounds good!

I think the whole brewery_type name came about because I ran into issues with type being reserved or something. Probably something automagic in Rails I wasn't using right.

@alexjustesen
Copy link
Contributor

@alexjustesen Sounds good!

I think the whole brewery_type name came about because I ran into issues with type being reserved or something. Probably something automagic in Rails I wasn't using right.

I think you're right, I'll switch it back

@chrisjm chrisjm mentioned this issue Mar 18, 2023
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 a pull request may close this issue.

2 participants