schedules: Use numeric ids for schedules tables primary keys #1086
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Work in ongoing to update the base classes/forms/communication, etc for numeric IDs instead of UUID. While doing this, some cleanup and simplifications will take place, so it is not a trivial work. |
kaligrafy
requested changes
Oct 21, 2024
| transaction?: Knex.Transaction; | ||
| } = {} | ||
| ): Promise<boolean> => { | ||
| if (typeof id === 'string' && !uuidValidate(id)) { |
There was a problem hiding this comment.
Can you add a comment saying that we now accept both uuids (string) or id (integer)?
There was a problem hiding this comment.
jsdoc was updated, also with the new options parameters
If a read/exists call is done within a transaction on an element that was added/modified during the transaction, if the transaction parameter is not set, the object will not be correctly read. This lets elements calls inside transaction to read the data as it is in the transaction.
The `save` function already covers both functionality more transparently than the caller having to select between `create` and `update`, so this should be called instead. But we can't remove it since currently the TransitObjectHandler use the create and update functions. But all other calls have been changed.
As some tables will migrate to numeric IDs and given that the knex queries automatically deal with parameter types, we can safely support either string or number IDs in the default DB functions. If it is a string, it is expected to be a uuid and we still validate its type.
Change the schedule, schedule_periods and schedule_period_trips's primary id columns from uuid to auto-increment numeric IDs. The old uuid is kept is a column called `uuid`. The foreign keys are modified to use the numeric key instead. The `schedule_id` column of the trips' table is removed, as there is already a schedule_period_id foreign which has a foreign key to the schedule_id. Having both in trips open the door to data integrity issues where the period could not belong to the same schedule as the trip. auto-increment numeric IDs are more performant than uuids as keys. It will also allow to more easily duplicate multiple elements in a table with predictable sort order, that uuids do not allow. This commit only changes the database table queries. The Schedule class still use the uuid as `id` as this would require more refactoring of the base classes, history tracker, forms and client/server messaging, that will be done in later commits. The numeric IDs are kept in the `integer_id` fields of the Schedule. The foreign key are changed to map to the numeric ID. The impact is "minimal" as the schedules are used mostly in calculations by trRouting and, in the frontend, are [re-]loaded on demand when changes occur on a specific line. There is no collection of this object type where the change of ID could cause issues.
tahini
force-pushed
the
dbSchedulesNumericId
branch
from
7e5546f
to
2832b24
Compare
kaligrafy
approved these changes
Oct 22, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Change the schedule, schedule_periods and schedule_period_trips's
primary id columns from uuid to auto-increment numeric IDs. The old
uuid is kept is a column called
uuid. The foreign keys are modifiedto use the numeric key instead. The
schedule_idcolumn of the trips'table is removed, as there is already a schedule_period_id foreign which
has a foreign key to the schedule_id. Having both in trips open the door
to data integrity issues where the period could not belong to the same
schedule as the trip.
auto-increment numeric IDs are more performant than uuids as keys. It
will also allow to more easily duplicate multiple elements in a table
with predictable sort order, that uuids do not allow.
This commit only changes the database table queries. The Schedule class
still use the uuid as
idas this would require more refactoring of thebase classes, history tracker, forms and client/server messaging, that
will be done in later commits.
The numeric IDs are kept in the
integer_idfields of the Schedule. Theforeign key are changed to map to the numeric ID.
The impact is "minimal" as the schedules are used mostly in calculations
by trRouting and, in the frontend, are [re-]loaded on demand when
changes occur on a specific line. There is no collection of this object
type where the change of ID could cause issues.