Database overhaul #60

New Issue

GiteaMirror · 2025-11-02T03:06:35-06:00

GiteaMirror commented

2025-11-02 03:06:35 -06:00

Originally created by @andreynering on GitHub (Nov 15, 2016).

Originally assigned to: @andreynering on GitHub.

Things to fix:

There are missing foreign keys
There are missing indexes on important columns:
- Foreign keys
- Timestamps

Things to discuss:

~~There are columns like num_issues, num_pulls, num_stars, etc. May those should just be a SELECT COUNT(*)?~~
Should we really store timestamps as unix in bigint columns? Seems like a bad practice to me. I think that was made because of timezone issues, since unix is automatically converted to UTC. Maybe we should change those back to datetime columns, but store time in UTC? Maybe @lunny can give any opinion on this.
Have a better database migration process?

Originally created by @andreynering on GitHub (Nov 15, 2016). Originally assigned to: @andreynering on GitHub. Things to fix: - [ ] There are missing foreign keys - [ ] There are missing indexes on important columns: - [ ] Foreign keys - [ ] Timestamps Things to discuss: - ~~There are columns like `num_issues`, `num_pulls`, `num_stars`, etc. May those should just be a `SELECT COUNT(*)`?~~ - ~~Should we really store timestamps as unix in bigint columns? Seems like a bad practice to me. I think that was made because of timezone issues, since unix is automatically converted to UTC. Maybe we should change those back to datetime columns, but store time in UTC? Maybe @lunny can give any opinion on this.~~ - Have a better database migration process?

GiteaMirror added the type/enhancement label 2025-11-02 03:06:35 -06:00

GiteaMirror closed this issue

2025-11-02 03:06:35 -06:00

GiteaMirror commented

2025-11-02 03:06:35 -06:00

@tboerger commented on GitHub (Nov 15, 2016):

I would prefer migrations like your gormigrate :)

But generally that sounds like a good idea overall.

@tboerger commented on GitHub (Nov 15, 2016): I would prefer migrations like your gormigrate :) But generally that sounds like a good idea overall.

GiteaMirror commented

2025-11-02 03:06:36 -06:00

@bkcsoft commented on GitHub (Nov 15, 2016):

Please do 1 model per PR. Otherwise it will be a biatch to review 😒

But I agree, DB really needs an overhaul 💯

I'd prefer if the models are made mockable at the same time, so we can add tests at the same time 😉

@bkcsoft commented on GitHub (Nov 15, 2016): Please do 1 model per PR. Otherwise it will be a _biatch_ to review :unamused: But I agree, DB really needs an overhaul :100: I'd prefer if the models are made mockable at the same time, so we can add tests at the same time :wink:

GiteaMirror commented

2025-11-02 03:06:36 -06:00

@andreynering commented on GitHub (Nov 15, 2016):

I would prefer migrations like your gormigrate :)

We are in need of a xormigrate 😃

Please do 1 model per PR. Otherwise it will be a biatch to review 😒

Good idea.

I'd prefer if the models are made mockable at the same time, so we can add tests at the same time 😉

I don't think that's possible, because models are to attached to fields, while Go interfaces only understands methods. It would be easier if we tested against a real DB.

@andreynering commented on GitHub (Nov 15, 2016): > I would prefer migrations like your gormigrate :) We are in need of a xormigrate :smiley: > Please do 1 model per PR. Otherwise it will be a biatch to review 😒 Good idea. > I'd prefer if the models are made mockable at the same time, so we can add tests at the same time 😉 I don't think that's possible, because models are to attached to fields, while Go interfaces only understands methods. It would be easier if we tested against a real DB.

GiteaMirror commented

2025-11-02 03:06:36 -06:00

@tboerger commented on GitHub (Nov 15, 2016):

Maybe these parts should be split, like drone, kleister or umschlag, there we got models and a store layer, e.g. https://github.com/kleister/kleister-api/tree/master/model and https://github.com/kleister/kleister-api/tree/master/store and https://github.com/kleister/kleister-api/tree/master/store/data

@tboerger commented on GitHub (Nov 15, 2016): Maybe these parts should be split, like drone, kleister or umschlag, there we got models and a store layer, e.g. https://github.com/kleister/kleister-api/tree/master/model and https://github.com/kleister/kleister-api/tree/master/store and https://github.com/kleister/kleister-api/tree/master/store/data

GiteaMirror commented

2025-11-02 03:06:36 -06:00

@lunny commented on GitHub (Nov 16, 2016):

@andreynering, I don't think every time to SELECT COUNT(*) is better than current implementation.
time.Unix() return int64, so we have to store it as big int, or we maybe lost something? In fact, xorm support big int in database and time.Time on struct automatically. I think we can optimize it later.
I think manually is safe enough. Of course, if some one have a better idea, please take it and we can discuss it.

@lunny commented on GitHub (Nov 16, 2016): @andreynering, I don't think every time to `SELECT COUNT(*)` is better than current implementation. `time.Unix()` return `int64`, so we have to store it as `big int`, or we maybe lost something? In fact, xorm support `big int` in database and `time.Time` on struct automatically. I think we can optimize it later. I think manually is safe enough. Of course, if some one have a better idea, please take it and we can discuss it.

GiteaMirror commented

2025-11-02 03:06:36 -06:00

@andreynering commented on GitHub (Nov 16, 2016):

@lunny

time.Unix() return int64, so we have to store it as big int

O meant store the time.Time type as a DATETIME column, instead of storing the result of time.Unix(). I thought on that because some database functionality won't work for Unix time, and maybe some day we have some reports like GitHub. But OK, that not the priority. 😄

Indexes and foreign keys are important, though.

@andreynering commented on GitHub (Nov 16, 2016): @lunny > time.Unix() return int64, so we have to store it as big int O meant store the `time.Time` type as a DATETIME column, instead of storing the result of `time.Unix()`. I thought on that because some database functionality won't work for Unix time, and maybe some day we have some reports like GitHub. But OK, that not the priority. :smile: Indexes and foreign keys are important, though.

GiteaMirror commented

2025-11-02 03:06:36 -06:00

@strk commented on GitHub (Nov 16, 2016):

Please consider migrations of deploys with values that would not pass new constraints. What would migrations do in that case ?

@strk commented on GitHub (Nov 16, 2016): Please consider migrations of deploys with values that would not pass new constraints. What would migrations do in that case ?

GiteaMirror commented

2025-11-02 03:06:37 -06:00

@lunny commented on GitHub (Nov 17, 2016):

@andreynering Both time.Time and time.Unix() are OK.
@strk migrations will be executed when you upgrade from lower version of Gitea/Gogs to newer version.

@lunny commented on GitHub (Nov 17, 2016): @andreynering Both `time.Time` and `time.Unix()` are OK. @strk migrations will be executed when you upgrade from lower version of Gitea/Gogs to newer version.

GiteaMirror commented

2025-11-02 03:06:37 -06:00

@bkcsoft commented on GitHub (Dec 2, 2016):

I'd prefer if the models are made mockable at the same time, so we can add tests at the same time 😉

I don't think that's possible, because models are to attached to fields, while Go interfaces only understands methods. It would be easier if we tested against a real DB.

@andreynering Well, we only mock the output of functions, so They still return the correct Struct, just using a mockable Interface for the functions. I'll see if I can link an example later on :)

@bkcsoft commented on GitHub (Dec 2, 2016): > > I'd prefer if the models are made mockable at the same time, so we can add tests at the same time 😉 > > I don't think that's possible, because models are to attached to fields, while Go interfaces only understands methods. It would be easier if we tested against a real DB. @andreynering Well, we only mock the output of functions, so They still return the correct Struct, just using a mockable Interface for the functions. I'll see if I can link an example later on :)

GiteaMirror commented

2025-11-02 03:06:37 -06:00

@bkcsoft commented on GitHub (Dec 2, 2016):

More notes on my prior statement, This isn't related to the struct overhaul, but the DB-functions themselfs (like func GetUserById(userID int64) => func (DBUsers users)GetById(id int64))

@bkcsoft commented on GitHub (Dec 2, 2016): More notes on my prior statement, This isn't related to the struct overhaul, but the DB-functions themselfs (like `func GetUserById(userID int64)` => `func (DBUsers users)GetById(id int64)`)

GiteaMirror referenced this issue

2025-11-02 11:39:19 -06:00

[PR #60] [MERGED] Show issue/PR number on title #15120

Sign in to join this conversation.

Branches Tags

1 Participants

Notifications

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: github-starred/gitea#60