-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
fix(example-todo-list): add steps to run in sql db #2448
Conversation
|
||
The Repository classes are used to bind the datasource and the models. Since | ||
we've changed to another datasource, we need to update the repositories which | ||
are located in the `src\repositories` folder. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
src/repositories
?
For details, see the [Database migrations](Database-migrations.md) documentation | ||
page. | ||
|
||
### Alter tables to define the foreign keys |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IIRC, postgresql
connector already supports foreign key constraints? Can you verify?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I ran the npm run migrate
command and it didn't create the foreign key constraints. Do I need to do something else?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Creating FK requires defining them in the model similar to MySQL. see
https://github.com/strongloop/loopback-connector-postgresql#auto-migrateauto-update-models-with-foreign-keys
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
let me give it a try tomorrow. Thanks @elv1s.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@raymondfeng @elv1s , I think I could use some help here. In todo.model.ts
, I'm trying to add the property settings that @elv1s mentioned above:
@property({
postgresql: {
foreignKeys: {
fk_todo_todoListId: {
name: 'fk_todo_todoListId',
entity: 'TodoList',
entityKey: 'id',
foreignKey: 'todoListId',
},
},
},
})
@belongsTo(() => TodoList)
todoListId: number;
However, it complained that I cannot apply more than one decorator:
Error: Decorator cannot be applied more than once on Todo.prototype.todoListId
when running npm run migrate
.
@elv1s, were you able to get it running? Am I missing something? Thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we need to remove the call to property()
decorator inside belongsTo()
so we can add property metadata like you've shown above @dhmlau (see #2256 (comment)).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@dhmlau The foreignKeys need to be defined in @model
.
Issues #2345, #2256, and PR #2442 are related to this in that the db field value of todoListId
cannot be updated when we already have a decorator associated to it. So if PK and FK are different types in the DB then creating the constraint will fail.
Also order of creation of the table is important. TodoList must exist when creating the Todo FK constraint.
Multiple decorators are not supported.
@model({
settings: {
foreignKeys: {
fk_todo_todoListId: {
name: 'fk_todo_todoListId',
entity: 'TodoList',
entityKey: 'id',
foreignKey: 'todoListId',
},
},
},
},
})
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@elv1s @raymondfeng , thanks for the info. I managed to get it working! :) Will work on the PR tomorrow.
|
||
```ts | ||
constructor( | ||
@inject('datasources.psqlds') dataSource: juggler.DataSource, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IIUC, as a result of this tutorial step, we end up with two datasources in our project:
db
datasource backed bymemory
connector - NOT USED BY ANY REPOpsqlds
datasource backed bypostgresql
connector - USED EVERYWHERE
I don't like the idea of having a datasource that's not used by any repository.
Also I consider it as a bad practice to name artifacts after implementation details. It's better to name them after our intention & their function. The datasource called db
is meant to be the primary data storage used by the application.
Can we find a way how to preserve db
datasource name please?
For example, we can tell users to simply change the configuration of the existing db
datasource to use postgresql
instead of memory
connector.
If we want a more interactive approach that will prompt them for PostgreSQL config, then perhaps we can tell them to rm src/datasources/db.datasource.*
and run lb4 datasource db
to re-create it with different config.
I think this PR is pretty much blocked by #2442 |
8bddb2e
to
e2d4a9b
Compare
Similar to the `Todo` model, we will specify the constraints in the `settings`. | ||
|
||
```ts | ||
@model({ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The FK definitions here seem the same as above ^
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
will fix it as @jannyHou suggested.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@dhmlau Nice doc! Added a few comments.
@model({ | ||
settings: { | ||
foreignKeys: { | ||
fk_todo_todoListId: { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
wouldn't this be fk_todoListImage_todoListId
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you're right. let me fix it.
The order of table creation is important. We are going to migrate `TodoList` | ||
model first and then `Todo` and `TodoListImage` models. | ||
|
||
1. We'll run the database migration on the `TodoList` table. In |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am wondering...why we don't run the migrate command only once with config models: ['TodoList', 'Todo', 'TodoListImage']
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would the migration happen in this order? If yes, that's even better. let me give it a try. thanks.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it is working! I've fixed my docs accordingly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM 🚢
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good 👍, just have some nitpicks (you can ignore them).
docs/site/sidebars/lb4_sidebar.yml
Outdated
@@ -70,6 +70,10 @@ children: | |||
url: todo-list-tutorial-controller.html | |||
output: 'web, pdf' | |||
|
|||
- title: 'Running on relational database' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Suggestion: on relational
-> on a relational
(same for the title
and summary
).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe running on relational databases
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sounds good!
|
||
If you are running this example using a relational database, there are extra | ||
steps in order to set up constraints in the database. We're using PostgreSQL for | ||
illustration, it would work for other relational databases in a similar way. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nitpick: it would...
-> but it would...
![database tables](todo-list-tutorial-dbtables.png) | ||
|
||
{% include note.html content=" | ||
There is an ongoing work on supporting strong relation with referential integrity. For details, please see [epic #2231](https://github.com/strongloop/loopback-next/issues/2331). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nitpick: There is an ongoing work
-> There is ongoing work
, relation
-> relations
We will use the `foreignKeys` attribute to determine the constraints in the | ||
database table. | ||
|
||
In `src\models\todo.model.ts`, add the `settings` options in the `@model` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
src/models/todo.model.ts
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oops. yes. thanks.
The order of table creation is important. We are going to migrate `TodoList` | ||
model before the `Todo` and `TodoListImage` models. | ||
|
||
In `src\migrate.ts`, modify this line: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
src/migrate.ts
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
@bajtos @raymondfeng, may I get your reviews again and see if I addressed your comments? Thanks. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks mostly good, I have few minor comments to consider.
No further review is necessary from my side.
}, | ||
}) | ||
export class Todo extends Entity { | ||
``` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's add little bit more code to make the snippet self-contained. At the moment, it's breaking syntax highlighting here on GitHub.
export class Todo extends Entity {
// etc.
}
}, | ||
}, | ||
}) | ||
export class TodoListImage extends Entity { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ditto.
export class TodoListImage extends Entity {
// etc.
}
```ts | ||
await app.migrateSchema({ | ||
existingSchema: existingSchema, | ||
models: ['TodoList', 'Todo', 'TodoListImage'], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are you sure this line is necessary? I believe that app.migrateSchema
is going to migrate ALL models by default. By omitting the list of models, we keep the code forward-compatible. In your proposal, developers adding new models in the future must remember to update the migration script too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From my experiment, this line is necessary. TodoList
table must be created first before Todo
and TodoListImage
. When I didn't specify the order, I will get an error about the TodoList table do not exist. I'm not sure what's the better way to specify the order but this change seems to be working for me.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Discussed with @bajtos. He suggested:
- Add comments in the above code snippet to explain why we need to do this. -- DONE
- Open a GH issue to investigate how we can improve the migration tool so that users don't need to explicitly specify the order. -- [Spike] Automigrate: no need to specify the order of tables to be migrated #2831 is created.
There are extra steps needed in order to run the todo-list application on relational database. This PR listed the steps.
Checklist
npm test
passes on your machinepackages/cli
were updatedexamples/*
were updated