Skip to content

[usage] Define db.Workspace model #10293

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

Merged
merged 2 commits into from
May 31, 2022
Merged

[usage] Define db.Workspace model #10293

merged 2 commits into from
May 31, 2022

Conversation

easyCZ
Copy link
Member

@easyCZ easyCZ commented May 27, 2022

Description

Define Workspace model in Golang.

Firstly, the model is generated from the table schema using

go install github.com/smallnest/gen@latest
gen \
	--sqltype=mysql \
	--connstr "<CONN_STRING>" \
	--database gitpod \
	--gorm \
	--table <table_name> \
	--overwrite \
	--json

It is then adjusted to correctly parse timestamps. In particular, we use VarChar for date times in our Typescript ORM model. This requires a custom serialization type (in this PR, it's VarcharTime) to correctly serialize.

This PR also adds a compatibility test with existing data by inserting the data Raw into the DB, and then querying it back to ensure the data matches.

Related Issue(s)

How to test

Unit tests

Release Notes

NONE

Documentation

NONE

@easyCZ easyCZ force-pushed the mp/db-workspace branch from 32d7193 to 58d8862 Compare May 31, 2022 07:11
@easyCZ easyCZ marked this pull request as ready for review May 31, 2022 07:11
}

func (n VarcharTime) String() string {
return time.Time(n).Format(time.RFC3339Nano)
Copy link
Member

Choose a reason for hiding this comment

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

This is a different format to what we currently store in the DB (2006-01-02T15:04:05.999999999Z07:00 vs. 2006-01-02T15:04:05.999Z) and wouldn't parse using JS new Date(<string>).

Copy link
Member Author

Choose a reason for hiding this comment

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

String() is only used for printing it out in a human readable form. It's the interface to Stringer() which printers use to serialize a struct. Given that the format used by JS is fairly non-standard, the human readable format here is the standard RFC3339.

There may be gotchas in other places with timestamps, and I'll be adding much more in regards to tests for those. For now, we're focused on reading records only so writes (while important) are not as a big of a deal at this point.

Copy link
Member Author

Choose a reason for hiding this comment

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

A secondary problem with what we store right now is that we abuse VarChar to store timestamps. We rely on somewhat obscure feature of MySQL which allows for VarChart timestamp comparisons to work. The same, for example, is not true in PostgreSQL

Copy link
Member

Choose a reason for hiding this comment

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

String() is only used for printing it out in a human readable form

@easyCZ Despite what format is standardized where, we should really thrive to have one format everywhere. Everything else will give us headaches when doing cross-service deugging.

Copy link
Member

@geropl geropl May 31, 2022

Choose a reason for hiding this comment

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

A secondary problem with what we store right now is that we abuse VarChar to store timestamps

While I admin that this solution disturbed my personally feeling of "this is not the way it should be done (sic)" I think it worked rather well.

We rely on somewhat obscure feature of MySQL which allows for VarChart timestamp comparisons to work

I think it's regular string ordering, and not that far fetched. 😉

The same, for example, is not true in PostgreSQL

True, but we would not translate the current scheme 1:1 to postgresql, anyway. Even if we were to do it today. 🙃

Copy link
Member

Choose a reason for hiding this comment

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

That being said, I'm fine with having this method returning the RFC3339 format. But let's please be careful to not confuse things in code, so we end up with reporting to different formats somewhere (so far there only ever has been one). 🙏

Copy link
Member Author

Choose a reason for hiding this comment

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

Usage of ISO 8601 is pretty non standard in Go. I'd strongly recommend that for human consumption, we stick with the standard way.

IMO, this is a non-issue in the bigger picture of this PR. But if it enables approval of this PR and allows us to progress usage based billing, I'll happily change it.

Copy link
Member Author

Choose a reason for hiding this comment

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

As promised, a follow-up to this issue is in a PR now: #10490 (with tests)

@easyCZ easyCZ requested a review from a team May 31, 2022 12:51
@github-actions github-actions bot added the team: webapp Issue belongs to the WebApp team label May 31, 2022
// Workspace represents the underlying DB object
type Workspace struct {
ID string `gorm:"primary_key;column:id;type:char;size:36;" json:"id"`
OwnerID string `gorm:"column:ownerId;type:char;size:36;" json:"owner_id"`
Copy link
Member

Choose a reason for hiding this comment

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

❓ Why the uncommon JSON naming scheme? 🤔

Copy link
Member Author

Choose a reason for hiding this comment

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

I didn't notice the JSON declaration here. It's what came from the code-generator. I'll update them to match column names.

Copy link
Member Author

Choose a reason for hiding this comment

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

In go, underscore case is the default for json fields so that's likely why the generator default to these.

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed.

Copy link
Member

@geropl geropl left a comment

Choose a reason for hiding this comment

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

Looks good, and thx for the tests!

/hold because of this comment: https://github.com/gitpod-io/gitpod/pull/10293/files#r885609211

I bet you have a good reason, and leave it to your judgement to adjust if possible (to avoid potential future confusion). 👍

@easyCZ
Copy link
Member Author

easyCZ commented May 31, 2022

/hold because of this comment: #10293 (files)

I bet you have a good reason, and leave it to your judgement to adjust if possible (to avoid potential future confusion). 👍

I'll add a follow-up to add additional tests for both reading and writing to ensure compatibility with existing data layer. I'll cross-reference this comment to an example printout and we can discuss further there.

I'll be landing this to unblock additional data models and progress usage based billing. Thank you for the comments and reviews!

@geropl
Copy link
Member

geropl commented May 31, 2022

I'll be landing this to unblock additional data models and progress usage based billing. Thank you for the comments and reviews!

Thank you @easyCZ ! 🙏

@easyCZ
Copy link
Member Author

easyCZ commented May 31, 2022

/unhold

@roboquat roboquat merged commit 1d4be66 into main May 31, 2022
@roboquat roboquat deleted the mp/db-workspace branch May 31, 2022 13:19
@roboquat roboquat added deployed: webapp Meta team change is running in production deployed Change is completely running in production labels Jun 9, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
deployed: webapp Meta team change is running in production deployed Change is completely running in production release-note-none size/XL team: webapp Issue belongs to the WebApp team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants