-
Notifications
You must be signed in to change notification settings - Fork 5
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
[1] Entity History Rework #310
base: main
Are you sure you want to change the base?
Conversation
{ | ||
"current": s.flatten(currentHistoryFieldsSchema), | ||
"previous": s.flatten(previousHistoryFieldsSchema), | ||
"entityData": s.flatten(entitySchema), | ||
} |
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.
Using flatten here with a transform below to the actual record was the best way I could think of creating this schema.
sql->Postgres.unsafe(`SELECT * FROM "public"."${mockEntityHistory.table.tableName}"`) | ||
|
||
describe("Entity history serde", () => { | ||
it("serializes and deserializes correctly", () => { |
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.
This test explains the desired effect/approach of using the schema for an entity
}) | ||
|
||
describe("Entity History Codegen", () => { | ||
it("Creates an insert function", () => { |
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.
This one just illustrates the insert function that gets generated
Assert.equal(expected, mockEntityHistory.createInsertFnQuery) | ||
}) | ||
|
||
Async.it("Creating tables and functions works", async () => { |
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.
This one demonstrates the function and how it will be called. Although each entity will have an actual function it can call in the next PR
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.
Really like the tests 👍
{ | ||
"current": s.flatten(currentHistoryFieldsSchema), | ||
"previous": s.flatten(previousHistoryFieldsSchema), | ||
"entityData": s.flatten(entitySchema), |
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 probably won't happen, but it might break if the entity has fields entity_history_log_index
etc
|
||
let createInsertFnQuery = { | ||
let insertFnName = `"insert_${table.tableName}"` | ||
let historRowArg = "history_row" |
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 historRowArg = "history_row" | |
let historyRowArg = "history_row" |
ORDER BY ${currentChangeFieldNames | ||
->Belt.Array.map(fieldName => fieldName ++ " DESC") | ||
->Js.Array2.joinWith(", ")} |
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 it should take the previous one even without the ORDER BY
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.
Postgres doesn't guarentee ordering say by insertion. So what we should probably do is have a serial field that increments and we can order by that instead. What do you think?
Reckon we will need that for the suggested unordered multichain implementation as well.
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.
Postgres doesn't guarentee ordering say by insertion.
Didn't know that.
So what we should probably do is have a serial field that increments and we can order by that instead. What do you think?
This sounds good, but the current approach also works
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 works for ordered multichain 😬
8643d95
to
63b4a28
Compare
f500b14
to
3ae5772
Compare
Next PR will apply the insert function on writes.