Conversation
WalkthroughThis set of changes removes the Changes
Sequence Diagram(s)sequenceDiagram
participant App
participant Persistence
participant PgStorage
participant EntityHistory
App->>Persistence: make()
Note right of Persistence: No cacheStorage used
App->>PgStorage: setOrThrow(sql, items, ~pgSchema, ~table, ~itemSchema)
App->>EntityHistory: fromTable(table, ~pgSchema, ~schema)
Possibly related PRs
Poem
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
codegenerator/cli/templates/static/codegen/src/LoadLayer.res (1)
57-57: Fix typo in comment.There's a typo in the comment: "registerign" should be "registering".
- // Since LoadManager.call prevents registerign entities already existing in the inMemoryStore, + // Since LoadManager.call prevents registering entities already existing in the inMemoryStore,
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
codegenerator/cli/npm/envio/src/Persistence.res(0 hunks)codegenerator/cli/npm/envio/src/db/EntityHistory.res(3 hunks)codegenerator/cli/npm/envio/src/db/Table.res(1 hunks)codegenerator/cli/templates/dynamic/codegen/src/TestHelpers_MockDb.res.hbs(1 hunks)codegenerator/cli/templates/dynamic/codegen/src/db/Entities.res.hbs(1 hunks)codegenerator/cli/templates/static/codegen/src/Config.res(0 hunks)codegenerator/cli/templates/static/codegen/src/Env.res(0 hunks)codegenerator/cli/templates/static/codegen/src/LoadLayer.res(2 hunks)codegenerator/cli/templates/static/codegen/src/LoadLayer.resi(0 hunks)codegenerator/cli/templates/static/codegen/src/db/TablesStatic.res(2 hunks)scenarios/test_codegen/test/helpers/Mock.res(1 hunks)scenarios/test_codegen/test/lib_tests/EntityHistory_test.res(1 hunks)scenarios/test_codegen/test/lib_tests/Persistence_test.res(0 hunks)scenarios/test_codegen/test/lib_tests/Table_test.res(0 hunks)
💤 Files with no reviewable changes (6)
- codegenerator/cli/templates/static/codegen/src/Env.res
- codegenerator/cli/templates/static/codegen/src/LoadLayer.resi
- codegenerator/cli/templates/static/codegen/src/Config.res
- codegenerator/cli/npm/envio/src/Persistence.res
- scenarios/test_codegen/test/lib_tests/Table_test.res
- scenarios/test_codegen/test/lib_tests/Persistence_test.res
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build_and_test
🔇 Additional comments (14)
codegenerator/cli/templates/dynamic/codegen/src/db/Entities.res.hbs (1)
107-107: LGTM! Clean refactoring to explicit schema handling.The change to pass
~pgSchema=Env.Db.publicSchemaexplicitly toEntityHistory.fromTableis a good improvement over embedding schema names in table definitions. This provides better separation of concerns and more explicit control over schema specification.scenarios/test_codegen/test/helpers/Mock.res (1)
57-57: Good simplification by removing unused logger parameter.Removing the
~loggerparameter from theloadEntitiesByIdsfunction signature cleans up the interface by eliminating an unused parameter. This aligns with the broader cleanup effort across the codebase.codegenerator/cli/templates/dynamic/codegen/src/TestHelpers_MockDb.res.hbs (1)
266-266: Consistent parameter cleanup improves interface design.The removal of the
~loggerparameter frommakeLoadEntitiesByIdsis consistent with similar changes across the codebase. This simplifies the function signature while maintaining all necessary functionality.codegenerator/cli/npm/envio/src/db/Table.res (1)
90-100: Verify consistency between PR objectives and actual changes.The removal of the
schemaNamefield from thetabletype and the~schemaNameparameter frommkTablerepresents a significant structural change to the table definition system. However, the PR title indicates "Just clean up, no changes" which seems inconsistent with these substantial modifications.Please verify that this breaking change aligns with the intended scope of this PR. The changes appear to be part of a larger refactoring to handle PostgreSQL schema names explicitly rather than embedding them in table definitions.
#!/bin/bash # Description: Check for any remaining usage of schemaName in table-related code # Expected: No remaining references to table.schemaName or ~schemaName parameters echo "Searching for remaining schemaName references in table-related files..." rg -A 3 -B 3 "schemaName" --type rescript echo "Searching for mkTable calls that might still use schemaName..." rg -A 5 "mkTable\(" --type rescriptLikely an incorrect or invalid review comment.
codegenerator/cli/templates/static/codegen/src/db/TablesStatic.res (2)
42-42: Good explicit schema reference in SQL query.Replacing the implicit
table.schemaNamereference with explicitEnv.Db.publicSchemain the SQL query makes the schema source more transparent and aligns with the broader refactoring to remove schema names from table definitions.
245-245: Consistent with schema handling refactoring.The explicit
~pgSchema=Env.Db.publicSchemaparameter toEntityHistory.fromTablefollows the same pattern established in the dynamic templates, providing consistent schema handling across the codebase.codegenerator/cli/templates/static/codegen/src/LoadLayer.res (2)
35-40: LGTM: Logger parameter removal is clean.The removal of the optional
~loggerparameter from theloadEntitiesByIdsfunction signature is consistent and simplifies the interface appropriately.
60-60: LGTM: Function call updated correctly.The call to
loadEntitiesByIdscorrectly reflects the updated function signature without the~loggerparameter.codegenerator/cli/npm/envio/src/db/EntityHistory.res (4)
178-178: LGTM: Explicit schema parameter improves clarity.Making the PostgreSQL schema an explicit parameter instead of relying on
table.schemaNameimproves the function's interface clarity and makes schema handling more explicit.
253-254: LGTM: Consistent use of explicit schema parameter.The schema-qualified table paths correctly use the explicit
pgSchemaparameter, ensuring consistent schema handling throughout the function.
176-176: ```shell
#!/bin/bashRefined search for literal eval( call sites in ReScript files
rg -n --fixed-strings "eval(" --glob "*.res"
--- `337-337`: ```shell #!/bin/bash # Display the full insertFnString definition to verify any dynamic content sed -n '320,360p' codegenerator/cli/npm/envio/src/db/EntityHistory.resscenarios/test_codegen/test/lib_tests/EntityHistory_test.res (2)
55-55: LGTM: Test updated for explicit schema parameter.The test correctly passes the explicit
pgSchema="public"parameter to match the updatedEntityHistory.fromTablefunction signature.
63-70: LGTM: Custom batch set function replaces removed utility.The custom
batchSetMockEntityfunction appropriately replaces the removedTable.PostgresInterop.makeBatchSetFnby directly callingPgStorage.setOrThrowwith the necessary parameters including the explicitpgSchema.
|
|
||
| let insertFn: (Postgres.sql, Js.Json.t, ~shouldCopyCurrentEntity: bool) => promise<unit> = | ||
| insertFnString->Table.PostgresInterop.eval | ||
| insertFnString->eval |
There was a problem hiding this comment.
@DZakh, I can't remember when this was added but don't you think eval should be at start up time and not at runtime of the insert function?
| //We need to update values here not delet the rows, since restarting without a row | ||
| //has a different behaviour to restarting with an initialised row with zero values | ||
| let resetCurrentCurrentSyncStateQuery = `UPDATE ${table.schemaName}.${table.tableName} | ||
| let resetCurrentCurrentSyncStateQuery = `UPDATE "${Env.Db.publicSchema}"."${table.tableName}" |
There was a problem hiding this comment.
Why make this change? I don't mind either way, and I see this file used to access the env anyhow, but it seems counter productive for librifying if we remove thes from the state/config of the table. I personally think ultimately the whole system should run off of a config outside of the environment and the generated registeration can apply the environment variables.
There was a problem hiding this comment.
I think I see the point in that, the PgStorage module is what accepts the public schema not at the table level. I wonder if this then shouldn't be moved to the PgStorage module.
There was a problem hiding this comment.
Yes. We should move it there at some point. Table shouldn't know anything about postgres implementation.
| let expected = `(sql, rows) => { | ||
| return sql\` | ||
| INSERT INTO "custom"."test_table" | ||
| \${sql(rows, "id", "field_a")} | ||
| ON CONFLICT(id) DO UPDATE | ||
| SET | ||
| "id" = EXCLUDED."id", "field_a" = EXCLUDED."field_a";\` | ||
| }` |
There was a problem hiding this comment.
Do these kinds of tests still exist somewhere else? I think I remember you made a new test suite for it.
There was a problem hiding this comment.
Yes, there are new tests. Also, it's weird to test code, which is not used :)
2 participants
Summary by CodeRabbit
Refactor
Tests
Chores