feat(postgres): default datetime/timestamp precision setting added #5311

fabis94 · 2022-08-24T19:38:32Z

fabis94 · 2022-08-24T19:47:03Z

I dunno why, but locally i'm getting random files that I never touched being changed and added to commits: lib/.gitignore and dialects/index.js

I've removed them (hence the multiple commits), but if I try to rebase the branch or anything they appear again. Is this some magic configured together with git hooks?

kibertoad · 2022-08-31T18:21:08Z

test/integration2/dialects/postgresql.spec.js

+} = require('../util/knex-instance-provider');
+
+describe('PostgreSQL dialect', () => {
+  const dbs = getAllDbs().filter((db) => db === Db.PostgresSQL);


so uh, why do you get a list and filter it down to a single value, instead of just starting with that single value?

good point, I was mostly just repeating this procedure the same way it was done in other integration tests

@kibertoad so should I just fix this part and then this is ready for merge?

let's try enabling tests for CockroachDB as well and see if that would also pass

@kibertoad I've refactored the tests to also test CockroachDb as well. The column alteration test has to be skipped though, because apparently CockroachDB doesn't support altering columns?

Considering that I don't know anything about CockroachDb, that this PR isn't even meant for it and I'm having to deal with CockroachDB specific issues now I'd prefer it if we can just release this feature as is and if anyone needs something extra for CockroachDB, they can always submit another PR.

test/integration2/dialects/postgresql.spec.js

coveralls · 2022-09-01T21:27:56Z

coverage: 92.789% (+0.002%) from 92.787%
when pulling 7ab78fa on fabis94:fabians/pg-default-precision
into 6243f6c on knex:master.

fabis94 · 2022-09-07T09:07:00Z

lib/.gitignore

@@ -7,4 +7,4 @@
 **/*.js.map

 # Do not include .js files from .ts files
-dialects\index.js
+dialects/index.js


this is done automatically by one of the npm scripts, in fact I can't even get rid of this change cause it gets added back every time I revert it

fabis94 · 2022-09-18T16:45:28Z

@kibertoad i'd appreciate it if we could get this merged sometime soon 🙌

fabis94 · 2022-09-22T10:35:39Z

@kibertoad I see some tests are failing, but the failures don't make sense to me - There are some connection refused errors and errors in tests that don't really seem to have anything to do with my changes, so could it be a test runner issue as well? The tests did run through successfully on my end

OlivierCavadenti · 2022-09-23T07:36:27Z

@kibertoad I see some tests are failing, but the failures don't make sense to me - There are some connection refused errors and errors in tests that don't really seem to have anything to do with my changes, so could it be a test runner issue as well? The tests did run through successfully on my end

I will check the tests later, but it's seems you do some strange init of DB instances...

fabis94 · 2022-09-23T07:41:17Z

@kibertoad I see some tests are failing, but the failures don't make sense to me - There are some connection refused errors and errors in tests that don't really seem to have anything to do with my changes, so could it be a test runner issue as well? The tests did run through successfully on my end

I will check the tests later, but it's seems you do some strange init of DB instances...

Thank you! This is a new & foreign repo for me, so I just tried to look at how other tests are written to write mine

fabis94 · 2022-09-30T08:21:54Z

@OlivierCavadenti @kibertoad Hi, any news on this? If there are any changes I need to do, let me know, but please also provide me some providers on the correct way to do things, cause this repo is new to me

fabis94 · 2022-10-10T13:33:30Z

Anything I can do to push this through quicker? I'm fine making changes, if anyone can give me pointers :)

rluvaton

left a few comments, great job!
let's resolve those and get this thing merged

@kibertoad what about docs update?

test/integration2/dialects/postgresCockroach.spec.js

fabis94 · 2023-12-05T08:44:52Z

@rluvaton I've resolved both of the issues you brought up

rluvaton

LGTM

@kibertoad can you please merge

kibertoad · 2023-12-07T18:29:04Z

@fabis94 Tests are failing

fabis94 · 2023-12-07T19:11:44Z

@kibertoad @rluvaton The test failures don't make sense to me, it's a bunch of "Error: connect ECONNREFUSED ::1:26257" errors, and I didn't change any of the connection settings so it doesn't feel very related 🤔 https://github.com/knex/knex/actions/runs/7098240971/job/19422356671

kibertoad · 2023-12-07T19:13:20Z

@fabis94 Only new tests are failing, though. See the end of logs at https://github.com/knex/knex/actions/runs/7098240971/job/19422352541?pr=5311

Are they passing locally for you?

rluvaton · 2023-12-07T20:22:02Z

test/integration2/dialects/postgresCockroach.spec.js

+          });
+
+          // cockroachdb doesn't support altering columns at this point in time? https://github.com/cockroachdb/cockroach/issues/49329
+          it('if default datetime precision specified, it gets used when altering datetime/timestamp fields', async () => {


Suggested change

it('if default datetime precision specified, it gets used when altering datetime/timestamp fields', async () => {

it('if default datetime precision specified, it gets used when altering datetime/timestamp fields', async function() {

rluvaton · 2023-12-07T20:26:43Z

It fails for me as well

fabis94 · 2023-12-11T08:48:19Z

@rluvaton @kibertoad This project doesn't exactly have the nicest contributor onboarding experience, the contributor docs are lacking and I was never able to get all of the tests working even on the main branch!

So yes, I do see test failures locally, but many of them seem unrelated to this ticket and I'd appreciate some help here considering that I don't know this project very well.

I fixed the issue @rluvaton brought up, but I'm still getting test failures locally:

 1) CLI tests
       knexfile
         knexfile resolution
           --cwd is NOT specified
             but --knexfile is specified
               changes the process's cwd to the directory that contains the knexfile:
     Error: Expected output to include "Working directory changed to ~/Code/random/knex/test/jake-util/knexfile-relative", but it was actually "Working directory changed to ~/Code/random/knex/test/jake-util/knexfile-relative
sqlite does not support inserting default values. Set the `useNullAsDefault` flag to hide this warning. (see docs https://knexjs.org/guide/query-builder.html#insert).
Batch 1 run: 2 migrations
"
      at new AssertionError (node_modules/cli-testlab/lib/AssertionError.ts:3:5)
      at assertOutput (node_modules/cli-testlab/lib/execution-test-helper.ts:71:14)
      at Exec.<anonymous> (node_modules/cli-testlab/lib/execution-test-helper.ts:149:9)
      at Exec.emit (node:events:513:28)
      at ChildProcess.<anonymous> (node_modules/cli-testlab/node_modules/jake/lib/utils/index.js:230:20)
      at ChildProcess.emit (node:events:513:28)
      at Process.ChildProcess._handle.onexit (node:internal/child_process:291:12)

  2) CLI tests
       knexfile
         knexfile resolution
           --cwd is NOT specified
             but --knexfile is specified
               and the knexfile itself resolves paths relative to process.cwd()
                 changes the process's cwd to the directory that contains the knexfile before opening the knexfile:
     Error: Expected output to include "Working directory changed to ~/Code/random/knex/test/jake-util/knexfile-relative", but it was actually "Working directory changed to ~/Code/random/knex/test/jake-util/knexfile-relative
sqlite does not support inserting default values. Set the `useNullAsDefault` flag to hide this warning. (see docs https://knexjs.org/guide/query-builder.html#insert).
Batch 1 run: 2 migrations
"
      at new AssertionError (node_modules/cli-testlab/lib/AssertionError.ts:3:5)
      at assertOutput (node_modules/cli-testlab/lib/execution-test-helper.ts:71:14)
      at Exec.<anonymous> (node_modules/cli-testlab/lib/execution-test-helper.ts:149:9)
      at Exec.emit (node:events:513:28)
      at ChildProcess.<anonymous> (node_modules/cli-testlab/node_modules/jake/lib/utils/index.js:230:20)
      at ChildProcess.emit (node:events:513:28)
      at Process.ChildProcess._handle.onexit (node:internal/child_process:291:12)

  3) PostgreSQL & Cockroach DB
       postgres
         when schema building
           with default datetime precision specified
             if default datetime precision specified, it gets used when creating datetime/timestamp fields:
     error: password authentication failed for user "testuser"
      at Parser.parseErrorMessage (node_modules/pg-protocol/src/parser.ts:369:69)
      at Parser.handlePacket (node_modules/pg-protocol/src/parser.ts:188:21)
      at Parser.parse (node_modules/pg-protocol/src/parser.ts:103:30)
      at Socket.<anonymous> (node_modules/pg-protocol/src/index.ts:7:48)
      at Socket.emit (node:events:513:28)
      at addChunk (node:internal/streams/readable:324:12)
      at readableAddChunk (node:internal/streams/readable:297:9)
      at Socket.Readable.push (node:internal/streams/readable:234:10)
      at TCP.onStreamRead (node:internal/stream_base_commons:190:23)

  4) PostgreSQL & Cockroach DB
       cockroachdb
         when schema building
           with default datetime precision specified
             if default datetime precision specified, it gets used when creating datetime/timestamp fields:
     error: create table "default_datetime_precision_test_1702284324708" ("timestamp_field" timestamptz(2), "datetime_field" timestamptz(2), "created_at" timestamptz(2), "updated_at" timestamptz(2)) - no database or schema specified
      at Parser.parseErrorMessage (node_modules/pg-protocol/src/parser.ts:369:69)
      at Parser.handlePacket (node_modules/pg-protocol/src/parser.ts:188:21)
      at Parser.parse (node_modules/pg-protocol/src/parser.ts:103:30)
      at Socket.<anonymous> (node_modules/pg-protocol/src/index.ts:7:48)
      at Socket.emit (node:events:513:28)
      at addChunk (node:internal/streams/readable:324:12)
      at readableAddChunk (node:internal/streams/readable:297:9)
      at Socket.Readable.push (node:internal/streams/readable:234:10)
      at TCP.onStreamRead (node:internal/stream_base_commons:190:23)

  5) PostgreSQL & Cockroach DB
       cockroachdb
         when schema building
           with default datetime precision specified
             if default datetime precision specified, it gets used when altering datetime/timestamp fields:
     error: create table "default_datetime_precision_test_1702284324713" ("timestamp_field" timestamptz(5), "datetime_field" timestamptz(5)) - no database or schema specified
      at Parser.parseErrorMessage (node_modules/pg-protocol/src/parser.ts:369:69)
      at Parser.handlePacket (node_modules/pg-protocol/src/parser.ts:188:21)
      at Parser.parse (node_modules/pg-protocol/src/parser.ts:103:30)
      at Socket.<anonymous> (node_modules/pg-protocol/src/index.ts:7:48)
      at Socket.emit (node:events:513:28)
      at addChunk (node:internal/streams/readable:324:12)
      at readableAddChunk (node:internal/streams/readable:297:9)
      at Socket.Readable.push (node:internal/streams/readable:234:10)
      at TCP.onStreamRead (node:internal/stream_base_commons:190:23)

rluvaton · 2023-12-12T12:13:37Z

Thank you for the feedback, do you mind opening an issue with all the thing you wish was better?

Are the with default datetime precision specified tests are failing in master as well?

feat(postgres): default datetime/timestamp precision setting added

d90a309

fabis94 mentioned this pull request Aug 24, 2022

Enable the configuring of the default precision for timestamp/datetime fields #5308

Open

fabis94 added 3 commits August 24, 2022 22:40

undoing unnecessary changes

32d93bb

undoing unnecessary changes

59acc2b

undoing unnecessary changes

1fe5b8a

kibertoad reviewed Aug 31, 2022

View reviewed changes

test/integration2/dialects/postgresql.spec.js Outdated Show resolved Hide resolved

fix: adding tests for cockroachdb

add291f

fabis94 commented Sep 7, 2022

View reviewed changes

fabis94 mentioned this pull request Sep 22, 2022

refactor(server): set an appropriate default timestamp precision for datetime/timestamp DB columns specklesystems/speckle-server#1019

Open

rluvaton reviewed Dec 2, 2023

View reviewed changes

test/integration2/dialects/postgresCockroach.spec.js Outdated Show resolved Hide resolved

test/integration2/dialects/postgresCockroach.spec.js Show resolved Hide resolved

fabis94 added 2 commits December 5, 2023 10:34

Merge branch 'knex:master' into fabians/pg-default-precision

42d8181

fix: minor code review fixes

c76004d

rluvaton approved these changes Dec 7, 2023

View reviewed changes

rluvaton reviewed Dec 7, 2023

View reviewed changes

test fix

7ab78fa

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(postgres): default datetime/timestamp precision setting added #5311

feat(postgres): default datetime/timestamp precision setting added #5311

fabis94 commented Aug 24, 2022

fabis94 commented Aug 24, 2022

kibertoad Aug 31, 2022 •

edited

fabis94 Aug 31, 2022

fabis94 Sep 1, 2022

kibertoad Sep 1, 2022

fabis94 Sep 7, 2022

coveralls commented Sep 1, 2022 •

edited

fabis94 Sep 7, 2022

fabis94 commented Sep 18, 2022

fabis94 commented Sep 22, 2022

OlivierCavadenti commented Sep 23, 2022

fabis94 commented Sep 23, 2022

fabis94 commented Sep 30, 2022

fabis94 commented Oct 10, 2022

rluvaton left a comment •

edited

fabis94 commented Dec 5, 2023

rluvaton left a comment

kibertoad commented Dec 7, 2023

fabis94 commented Dec 7, 2023

kibertoad commented Dec 7, 2023

rluvaton Dec 7, 2023

rluvaton commented Dec 7, 2023

fabis94 commented Dec 11, 2023

rluvaton commented Dec 12, 2023

	it('if default datetime precision specified, it gets used when altering datetime/timestamp fields', async () => {
	it('if default datetime precision specified, it gets used when altering datetime/timestamp fields', async function() {

feat(postgres): default datetime/timestamp precision setting added #5311

Are you sure you want to change the base?

feat(postgres): default datetime/timestamp precision setting added #5311

Conversation

fabis94 commented Aug 24, 2022

fabis94 commented Aug 24, 2022

kibertoad Aug 31, 2022 • edited

Choose a reason for hiding this comment

fabis94 Aug 31, 2022

Choose a reason for hiding this comment

fabis94 Sep 1, 2022

Choose a reason for hiding this comment

kibertoad Sep 1, 2022

Choose a reason for hiding this comment

fabis94 Sep 7, 2022

Choose a reason for hiding this comment

coveralls commented Sep 1, 2022 • edited

fabis94 Sep 7, 2022

Choose a reason for hiding this comment

fabis94 commented Sep 18, 2022

fabis94 commented Sep 22, 2022

OlivierCavadenti commented Sep 23, 2022

fabis94 commented Sep 23, 2022

fabis94 commented Sep 30, 2022

fabis94 commented Oct 10, 2022

rluvaton left a comment • edited

Choose a reason for hiding this comment

fabis94 commented Dec 5, 2023

rluvaton left a comment

Choose a reason for hiding this comment

kibertoad commented Dec 7, 2023

fabis94 commented Dec 7, 2023

kibertoad commented Dec 7, 2023

rluvaton Dec 7, 2023

Choose a reason for hiding this comment

rluvaton commented Dec 7, 2023

fabis94 commented Dec 11, 2023

rluvaton commented Dec 12, 2023

kibertoad Aug 31, 2022 •

edited

coveralls commented Sep 1, 2022 •

edited

rluvaton left a comment •

edited