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

Add postgres adapter #96

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Add postgres adapter #96

wants to merge 2 commits into from

Conversation

aguspdana
Copy link
Collaborator

@aguspdana aguspdana commented Feb 3, 2025

Important

Add PostgreSQL adapter with client support, configuration, schema, data, and tests, including PostgreSQL-specific query handling and limitations.

  • PostgreSQL Adapter:
    • Add PostgreSQL client support in src/index.ts and queryMachine.ts.
    • Implement PostgreSQL-specific query building and execution in pg/build.ts, pg/machine.ts, and pg/run.ts.
  • Configuration:
    • Add PostgreSQL configuration in tests/adapters/postgresDB/mocks/config.ts.
    • Update init.ts to include PostgreSQL test configuration.
  • Schema and Data:
    • Create PostgreSQL schema in schema.sql and data in data.sql.
  • Testing:
    • Add postgres.sh for setting up PostgreSQL test environment.
    • Modify query.ts to include PostgreSQL-specific test cases and handle unsupported features.
    • Update package.json to include PostgreSQL dependencies and test scripts.
  • Miscellaneous:
    • Update schema.ts to include PostgreSQL-specific configurations for roles and fields.

This description was created by Ellipsis for 288fd2a. It will automatically update as commits are pushed.

Copy link
Contributor

@ellipsis-dev ellipsis-dev bot left a comment

Choose a reason for hiding this comment

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

❌ Changes requested. Reviewed everything up to 725b90c in 3 minutes and 50 seconds

More details
  • Looked at 3476 lines of code in 19 files
  • Skipped 0 files when reviewing.
  • Skipped posting 26 drafted comments based on config settings.
1. src/index.ts:50
  • Draft comment:
    Good initialization: the new PostgresDB handle is added to dbHandles with a cleaner Promise.all approach.
  • Reason this comment was not posted:
    Confidence changes required: 0%
    None
2. src/index.ts:141
  • Draft comment:
    Solid query pipeline implementation via runQueryMachine and error handling with tryit().
  • Reason this comment was not posted:
    Confidence changes required: 0%
    None
3. src/stateMachine/query/pg/README.md:1
  • Draft comment:
    Documentation for SQL syntax and examples is clear and useful.
  • Reason this comment was not posted:
    Confidence changes required: 0%
    None
4. src/stateMachine/query/pg/build.ts:269
  • Draft comment:
    Nice use of aliases and error-checks in role field query construction. Ensure that throwing error on mismatched FK lengths is sufficient.
  • Reason this comment was not posted:
    Confidence changes required: 0%
    None
5. src/stateMachine/query/pg/machine.ts:87
  • Draft comment:
    State machine for Postgres query processing integrates error transitions consistently.
  • Reason this comment was not posted:
    Confidence changes required: 0%
    None
6. src/stateMachine/query/pg/run.ts:33
  • Draft comment:
    SQL string building via useOrdinalPlaceholder is neatly implemented; ensure that parameter indexing works correctly under all conditions.
  • Reason this comment was not posted:
    Confidence changes required: 20%
    None
7. src/stateMachine/query/queryMachine.ts:134
  • Draft comment:
    Adapter selection includes PostgresDB. Ensure that the indices pushed and reordering of results match the original query order.
  • Reason this comment was not posted:
    Confidence changes required: 10%
    None
8. src/types/config/base.ts:32
  • Draft comment:
    Provider union now includes PostgresDBProvider; documentation and type definitions appear consistent.
  • Reason this comment was not posted:
    Confidence changes required: 0%
    None
9. src/types/config/postgres.ts:4
  • Draft comment:
    PostgresDBProvider interface is minimalistic and seems sufficient for connection info.
  • Reason this comment was not posted:
    Confidence changes required: 0%
    None
10. src/types/schema/fields.ts:19
  • Draft comment:
    Addition of 'dbConfig' to RoleField supports Postgres mapping; dbPath is now in DataField; this integration appears sound.
  • Reason this comment was not posted:
    Confidence changes required: 0%
    None
11. tests/adapters/postgresDB/mocks/config.ts:3
  • Draft comment:
    Postgres test config is straightforward; consider adding comment regarding test DB cleanup if necessary.
  • Reason this comment was not posted:
    Confidence changes required: 0%
    None
12. tests/adapters/postgresDB/mocks/data.sql:29
  • Draft comment:
    SQL data inserted for testing; check that comments about multiple keys match actual primary key constraints in schema.
  • Reason this comment was not posted:
    Confidence changes required: 0%
    None
13. tests/adapters/postgresDB/mocks/schema.sql:65
  • Draft comment:
    Schema for Postgres reflects necessary tables; review comment in Color table (freeForAll) for potential inconsistencies.
  • Reason this comment was not posted:
    Confidence changes required: 0%
    None
14. tests/helpers/init.ts:14
  • Draft comment:
    Adapter selection for tests now includes PostgresDB; simplicity is maintained.
  • Reason this comment was not posted:
    Confidence changes required: 0%
    None
15. tests/mocks/schema.ts:35
  • Draft comment:
    Test schema includes linkFields and hooks; be sure that inherited features not supported in Postgres are marked with TODO in tests.
  • Reason this comment was not posted:
    Confidence changes required: 0%
    None
16. tests/multidb/mocks/schema.tql:10
  • Draft comment:
    TQL schema remains unchanged; ensure that its usage is isolated to typeDB tests as intended.
  • Reason this comment was not posted:
    Confidence changes required: 0%
    None
17. tests/postgres.sh:1
  • Draft comment:
    Bash script to run Postgres tests is clear; consider adding more robust waiting (e.g., checking logs) if tests are flaky.
  • Reason this comment was not posted:
    Confidence changes required: 20%
    None
18. tests/unit/queries/query.ts:26
  • Draft comment:
    Many tests are tagged with TODO{P} or TODO{PTS} indicating known limitations on Postgres side. Ensure clear documentation in issue tracker for these limitations.
  • Reason this comment was not posted:
    Confidence changes required: 0%
    None
19. tests/unit/queries/query.ts:1247
  • Draft comment:
    Test for local filters and nested queries is detailed; ensure that deep sorting and metadata removal functions handle Postgres outputs correctly.
  • Reason this comment was not posted:
    Confidence changes required: 10%
    None
20. tests/unit/queries/query.ts:1634
  • Draft comment:
    Tests for inheritance (ex1, ex2) marked TODO{P} indicate workarounds. Verify that these workarounds are well documented in the code and in the issues list.
  • Reason this comment was not posted:
    Confidence changes required: 20%
    None
21. tests/unit/queries/query.ts:1916
  • Draft comment:
    Test for computed virtual field co2 is marked as TODO; note that computed fields are not yet supported for Postgres. This comment is useful.
  • Reason this comment was not posted:
    Confidence changes required: 0%
    None
22. tests/unit/queries/query.ts:1975
  • Draft comment:
    Tests for multiVal, query mv1/mv2 use TODO{PT} tags; ensure that the differences in flex type output between TypeDB and Postgres are clearly documented.
  • Reason this comment was not posted:
    Confidence changes required: 0%
    None
23. tests/unit/queries/query.ts:2067
  • Draft comment:
    Tests for includes operators on cardinality ONE are expected to throw errors; consider catching these errors explicitly in tests to improve clarity.
  • Reason this comment was not posted:
    Confidence changes required: 30%
    None
24. tests/unit/queries/query.ts:2083
  • Draft comment:
    There is a logical inconsistency in the expected result of the '[entity,filter,not]' test. The second object has both $id and id mismatched (it shows $id: 'user2' with id: 'user3'). This appears to be a copy‐paste error; the expected $id for the second object should likely be 'user3'.
  • Reason this comment was not posted:
    Comment was not on a location in the diff, so it can't be submitted as a review comment.
25. tests/unit/queries/query.ts:2066
  • Draft comment:
    Tests expecting errors (e.g. using array filter operators on cardinality ONE) are using 'expect(res).toThrow(...)' on an awaited promise. For async functions that reject, use 'await expect(ctx.query(...)).rejects.toThrow(...)' to properly capture the rejection.
  • Reason this comment was not posted:
    Comment was not on a location in the diff, so it can't be submitted as a review comment.
26. tests/unit/queries/query.ts:1078
  • Draft comment:
    There is mixed usage of the 'noMetadata' option in queries. Some tests compare full responses including metadata while others pass { noMetadata: true } and compare simplified objects. Consider standardizing expected outputs (or using helper functions) so that tests remain consistent across different modes.
  • Reason this comment was not posted:
    Decided after close inspection that this draft comment was likely wrong and/or not actionable:
    This is a valid code quality observation about test consistency. Having mixed styles makes the tests harder to maintain and understand. The comment suggests a concrete improvement (standardizing the approach) and points out a real issue. However, it's not directly related to any changes in the diff - the inconsistency existed before and wasn't introduced or modified by these changes.
    The comment identifies a real issue but may be out of scope since it's about pre-existing test patterns rather than changes in this PR. It could also be argued that having flexibility in test styles is acceptable.
    While the scope concern is valid, test consistency is important for maintainability. The fact that this pattern is widespread in the tests makes it more important to address, not less. Having a standard approach would make the tests clearer.
    Delete the comment. While it raises a valid point about test consistency, it's addressing a pre-existing pattern rather than changes introduced in this PR. This type of broader refactoring suggestion should be tracked separately.

Workflow ID: wflow_DOvm9UeDejKZ69Vv


Want Ellipsis to fix these issues? Tag @ellipsis-dev in a comment. You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.

@@ -23,7 +23,8 @@ export const testQuery = createTest('Query', (ctx) => {
await expect(res).toBeNull();
});

it('e1[entity] - basic and direct link to relation', async () => {
it('TODO{P}:e1[entity] - basic and direct link to relation', async () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Many tests are prefixed with TODO markers (e.g. TODO{P}, TODO{PTS}) indicating known limitations for the Postgres adapter. Consider using conditional skips (e.g. it.skip) for tests unsupported for Postgres, so that the test suite can run cleanly without failing these known issues.

Copy link
Contributor

@ellipsis-dev ellipsis-dev bot left a comment

Choose a reason for hiding this comment

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

👍 Looks good to me! Incremental review on 288fd2a in 1 minute and 42 seconds

More details
  • Looked at 643 lines of code in 5 files
  • Skipped 0 files when reviewing.
  • Skipped posting 18 drafted comments based on config settings.
1. tests/unit/queries/query.ts:26
  • Draft comment:
    Test ‘TODO{P}:e1[entity]’ uses hardcoded expected results; ensure that the ordering and metadata conventions match the cache‐mirror conventions.
  • Reason this comment was not posted:
    Comment was not on a location in the diff, so it can't be submitted as a review comment.
2. tests/unit/queries/query.ts:168
  • Draft comment:
    Test ‘TODO{P}:e3[entity, nested]’ includes inherited entities (God/SuperUser) which are unsupported on Postgres; ensure these edge cases are marked clearly.
  • Reason this comment was not posted:
    Comment was not on a location in the diff, so it can't be submitted as a review comment.
3. tests/unit/queries/query.ts:568
  • Draft comment:
    For test ‘TODO{P}:r4[relation, nested, direct]’, verify that the expected nested structure for Room bookings and guests is consistent with our recursive atom cache updates.
  • Reason this comment was not posted:
    Comment was not on a location in the diff, so it can't be submitted as a review comment.
4. tests/unit/queries/query.ts:688
  • Draft comment:
    Test ‘TODO{ST}:r5.alt[relation nested]’ uses alternative expected output; verify that both role and linkfield usage of the same role are consistent with our atom cache design.
  • Reason this comment was not posted:
    Comment did not seem useful.
5. tests/unit/queries/query.ts:706
  • Draft comment:
    Test ‘TODO{P}:r6[relation nested]’ checks relation connected to relation and tunneled relation. Review that nested fields (booking, guest) are correctly driven via the atom tree and not react query cache fields.
  • Reason this comment was not posted:
    Comment did not seem useful.
6. tests/unit/queries/query.ts:773
  • Draft comment:
    Test ‘TODO{P}:r7[relation, nested, direct]’ – Check that nested on nested queries (with roles and link fields) correctly exclude inherited entities as per Postgres limitations.
  • Reason this comment was not posted:
    Comment did not seem useful.
7. tests/unit/queries/query.ts:860
  • Draft comment:
    Test ‘TODO{P}:ef3[entity] - $fields single’ includes inherited attributes not supported on Postgres; verify that the test expectation is adjusted according to our cache update rules.
  • Reason this comment was not posted:
    Comment was not on a location in the diff, so it can't be submitted as a review comment.
8. tests/unit/queries/query.ts:2304
  • Draft comment:
    Test ‘TODO{P}:a1[$as]’ uses field aliasing on nested attributes. Verify that aliases are applied on both attributes and nested roles according to atom cache rules.
  • Reason this comment was not posted:
    Comment was not on a location in the diff, so it can't be submitted as a review comment.
9. tests/unit/queries/query.ts:2356
  • Draft comment:
    Test ‘TODO{ST}:a1.alt[$as]’ for Booking uses $as alias for nested roles; ensure consistency with our cache mirror that always reads atoms.
  • Reason this comment was not posted:
    Comment did not seem useful.
10. tests/unit/queries/query.ts:2467
  • Draft comment:
    Test ‘TODO{P}:dn1[deep nested]’ – Review this deeply nested query. Consider splitting into smaller test units if possible for maintainability.
  • Reason this comment was not posted:
    Comment was not on a location in the diff, so it can't be submitted as a review comment.
11. tests/unit/queries/query.ts:2904
  • Draft comment:
    Test ‘TODO{ST}:dn1.alt[deep nested]’ uses JSON parsing workaround. Consider whether the JSON.stringify/parse is necessary and if deepSort function can handle circular references.
  • Reason this comment was not posted:
    Decided after close inspection that this draft comment was likely wrong and/or not actionable:
    The comment raises a valid point about potentially unnecessary JSON serialization/deserialization. However, this appears to be a test file, and the JSON.stringify/parse pattern is likely used intentionally to handle circular references or ensure deep cloning. The comment is speculative ("Consider whether...") and asks for investigation rather than pointing out a clear issue.
    The comment identifies a real code pattern that could be worth investigating. JSON serialization can impact performance.
    However, the comment is speculative and asks for investigation rather than identifying a clear problem. In test code, JSON serialization patterns are often used intentionally for handling test data.
    The comment should be deleted because it is speculative and asks for investigation rather than identifying a clear issue that needs to be fixed. The JSON.stringify/parse usage is likely intentional in this test context.
12. tests/unit/queries/query.ts:3038
  • Draft comment:
    Test ‘TODO{P}:fk2[filter, keywords, exists]’ – Ensure that behavior when filtering on null/undefined properties is consistent with our atom-based cache rules, especially given differences with Surreal/TypeDB.
  • Reason this comment was not posted:
    Comment was not on a location in the diff, so it can't be submitted as a review comment.
13. tests/unit/queries/query.ts:3122
  • Draft comment:
    The overall coverage in this file is extensive. Consider splitting tests into logical groups or test files for maintainability.
  • Reason this comment was not posted:
    Confidence changes required: 50%
    None
14. tests/unit/queries/query.ts:26
  • Draft comment:
    Several tests are marked with TODO (eg. TODO{P}, TODO{ST}, TODO{PTS}). Consider using a skip mechanism (e.g. test.skip) for features not yet supported to avoid false negatives in CI.
  • Reason this comment was not posted:
    Comment was not on a location in the diff, so it can't be submitted as a review comment.
15. tests/unit/queries/query.ts:2909
  • Draft comment:
    For deeply nested queries (eg. in r8.alt and dn1.alt), you use JSON.parse(JSON.stringify(res)) to strip metadata. Consider abstracting this into a helper function to improve clarity and maintainability.
  • Reason this comment was not posted:
    Marked as duplicate.
16. tests/unit/queries/query.ts:100
  • Draft comment:
    This test file is very long and covers many query types. Consider grouping tests into separate describe blocks or splitting them into multiple files (e.g. entity queries, relation queries, nested queries) for easier maintenance and readability.
  • Reason this comment was not posted:
    Comment was not on a location in the diff, so it can't be submitted as a review comment.
17. tests/unit/queries/query.ts:1614
  • Draft comment:
    Test 'lf2[$filter, $not]' uses a filter on a link field with cardinality ONE. Ensure that the expected behavior (excluding records with user 'user1') remains consistent across adapters and document this behavior clearly.
  • Reason this comment was not posted:
    Comment was not on a location in the diff, so it can't be submitted as a review comment.
18. tests/unit/queries/query.ts:1047
  • Draft comment:
    Some queries return a single object while others return an array when filtering by unique fields. It would be beneficial to clarify and document this behavior to ensure consistency across adapters.
  • Reason this comment was not posted:
    Confidence changes required: 50%
    None

Workflow ID: wflow_YV9O1UhPjsd6G9lm


You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.

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.

1 participant