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

Object lists package #230

Merged
merged 26 commits into from
Sep 21, 2023
Merged

Object lists package #230

merged 26 commits into from
Sep 21, 2023

Conversation

barbarah
Copy link
Contributor

@barbarah barbarah commented Sep 10, 2023

This is the object list package without a front end. This pull request contains the connection and configuration of the SQL database with some basic first functions to communicate with the database.

Things to discuss

Some topics still need some discussion.

1. The name of the package and database

I need some help naming the package and database. This is mainly because I do not know the scope of the database. The package is the connector to the database with some helper functions to communicate with the database.

Is this database only going to contain object lists? Then object-lists is probably a good name. If the scope of the database grows, the name needs to be corrected. The same goes for the database in PlanetScale; this is likely the same or similar name to this package. For now, the name is research-app within the colonial-collections organization. Should it be called object-lists or user-input? I need more information about the scope to decide on the naming.

2. Database branches and migrations

For now, we have chosen to use a free PlanetScale database until we have decided on the location (AWS?). Because there is a significant change in the database is going to be move, I wanted to keep the migrations simple but good enough. Also, the free version of PlanetScale is limited to one database with two branches. If we go for a paid plan, we could create a branch per preview environment. But for now, I created one main branch for production and one development branch for preview and development.

Creating a pull request with file changes to the migrations folder will trigger a migration to the development database. Merging the pull request will trigger a database merge from development to production. The GitHub action is based on an official PlanetScale blog post.

4. Rollbacks

To keep things simple for this temporary database, I suggest manual rollbacks. If the development database needs a rollback, we could make a new branch from production and use the new branch. For production, there are two options. After the merge, there is a button in PlanetScale to undo the merge. This button is only visible for 30 minutes, so if we notice within 30 minutes a rollback is necessary, we could push the button and let PlanetScale figure it out. After 30 minutes, we need to write a new migration. Because we do not have a complex database for now, I think this is good enough but something to consider when choosing the final database location.

3. Testing

I have been thinking about testing this package. In a drizzle discussion, the advice is not to write unit tests but rather focus on the right and few integration tests. I have written some cypress tests for the front-end. But this is not part of this pull request. I moved the front-end to a separate pull request because there was still some discussion about the user interface. So, those tests are also not in this pull request.

@vercel
Copy link

vercel bot commented Sep 10, 2023

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
colonial-collections-dataset-browser ✅ Ready (Inspect) Visit Preview 💬 Add feedback Sep 21, 2023 0:54am
colonial-collections-researcher ✅ Ready (Inspect) Visit Preview 💬 Add feedback Sep 21, 2023 0:54am

@barbarah barbarah marked this pull request as draft September 10, 2023 13:57
@github-actions
Copy link

github-actions bot commented Sep 10, 2023

Database schema changes have been applied

The development database migrated with the migrations found in this branch.

Warning: All preview and development environments use the same database. This migration can affect all preview and development environments.

After merging this pull request, the development database changes will merge into the production database.

.github/workflows/database-merge-schema-changes.yml Outdated Show resolved Hide resolved
.github/workflows/database-merge-schema-changes.yml Outdated Show resolved Hide resolved
.github/workflows/database-merge-schema-changes.yml Outdated Show resolved Hide resolved
.github/workflows/database-merge-schema-changes.yml Outdated Show resolved Hide resolved
.github/workflows/database-merge-schema-changes.yml Outdated Show resolved Hide resolved
packages/object-lists/src/db/connection.ts Outdated Show resolved Hide resolved
packages/object-lists/src/db/connection.ts Outdated Show resolved Hide resolved
packages/object-lists/src/db/schema.ts Outdated Show resolved Hide resolved
packages/object-lists/tsconfig.json Outdated Show resolved Hide resolved
packages/object-lists/tsconfig.json Outdated Show resolved Hide resolved
@barbarah barbarah force-pushed the object-lists-package branch from 0b7bb74 to c7b8041 Compare September 13, 2023 11:41
@barbarah barbarah force-pushed the object-lists-package branch from c7b8041 to 2d1968a Compare September 13, 2023 11:48
@barbarah barbarah force-pushed the object-lists-package branch from 2d1968a to 5d93ade Compare September 13, 2023 11:52
@barbarah barbarah force-pushed the object-lists-package branch from f81002d to 3c4fcdb Compare September 14, 2023 14:52
@barbarah barbarah force-pushed the object-lists-package branch from b9662ea to 8ce7fb5 Compare September 16, 2023 08:57
@barbarah barbarah marked this pull request as ready for review September 16, 2023 09:26
@barbarah barbarah requested a review from sdevalk September 16, 2023 09:31
README.md Show resolved Hide resolved
packages/datahub-data/README.md Outdated Show resolved Hide resolved
packages/datahub-data/README.md Outdated Show resolved Hide resolved
packages/datahub-data/README.md Outdated Show resolved Hide resolved
packages/datahub-data/drizzle.config.ts Outdated Show resolved Hide resolved
packages/datahub-data/package.json Outdated Show resolved Hide resolved
packages/datahub-data/src/db/schema.ts Outdated Show resolved Hide resolved
packages/datahub-data/src/object-list.ts Outdated Show resolved Hide resolved
packages/datahub-data/src/object-list.ts Outdated Show resolved Hide resolved
packages/object-lists/src/db/schema.ts Outdated Show resolved Hide resolved
@barbarah
Copy link
Contributor Author

barbarah commented Sep 20, 2023

I have moved the migration to a separate script instead of using drizzle-kit push:mysql. Because an unsuccessful run of drizzle-kit is still resolved in a successful turbo task and a successful GitHub action. I want an unsuccessful GitHub action if the migrations fail.

packages/database/README.md Outdated Show resolved Hide resolved
packages/database/README.md Outdated Show resolved Hide resolved
packages/database/README.md Outdated Show resolved Hide resolved
packages/database/README.md Outdated Show resolved Hide resolved
packages/database/README.md Show resolved Hide resolved
packages/database/src/db/connection.ts Outdated Show resolved Hide resolved
packages/database/src/index.ts Outdated Show resolved Hide resolved
packages/database/src/iriToHash.test.ts Outdated Show resolved Hide resolved
packages/database/src/iriToHash.test.ts Outdated Show resolved Hide resolved
packages/database/src/iriToHash.ts Outdated Show resolved Hide resolved
packages/database/README.md Outdated Show resolved Hide resolved
packages/database/drizzle.config.ts Outdated Show resolved Hide resolved
CONSTRAINT `object_item_id_object_list_id_unique` UNIQUE(`id`,`object_list_id`)
);
--> statement-breakpoint
CREATE INDEX `id` ON `object_item` (`id`);--> statement-breakpoint
Copy link
Contributor

Choose a reason for hiding this comment

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

Mmm - I just realized this:

  1. The id is not unique. But the name id could give that impression (since the id in object_list is unique). So the meaning of id in this column differs from the same column in the other table.
  2. If we ever want to add a 'true' id column (i.e. similar to the id column in object_list) then the current column stands in the way - the name is already taken.
  3. Would it then be better to rename it to e.g. object_id (reasoning: it's the ID that we've given to the object - it's not the ID of the object_item record)? And to rename object_id to object_iri (I don't like the _iri suffix, but I don't have an alternative)?

Sorry for the hassle!

packages/database/scripts/create-database.sh Outdated Show resolved Hide resolved
packages/database/scripts/create-database.sh Outdated Show resolved Hide resolved
packages/database/scripts/create-database.sh Outdated Show resolved Hide resolved
packages/database/scripts/create-database.sh Outdated Show resolved Hide resolved
packages/database/scripts/migrate.ts Outdated Show resolved Hide resolved
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.

2 participants