activerecord-cockroachdb-adapter icon indicating copy to clipboard operation
activerecord-cockroachdb-adapter copied to clipboard

fix(schema_statements): add unique_rowid to pk_and_sequence_for

Open BuonOmo opened this issue 1 year ago • 1 comments

Allow to get the primary key in the default case.

This fixes two of the four failing test_pk_and_sequence_for*. The two still failing are:

test_pk_and_sequence_for_returns_nil_if_no_pk

      def test_pk_and_sequence_for_returns_nil_if_no_pk
        with_example_table "id integer" do
          assert_nil @connection.pk_and_sequence_for("ex") # => `["rowid", nil]`
        end
      end

the example table creation query:

CREATE TABLE ex(id integer)

The second query of #pk_and_sequence_for returns ["rowid", "public", nil]. And the SHOW CREATE TABLE ex confirms it:

CREATE TABLE public.ex (
	id INT8 NULL,
	rowid INT8 NOT VISIBLE NOT NULL DEFAULT unique_rowid(),
	CONSTRAINT ex_pkey PRIMARY KEY (rowid ASC)
)

test_pk_and_sequence_for_with_collision_pg_class_oid

create table ex(id serial primary key);
create table ex2(id serial primary key);
DELETE FROM pg_depend WHERE objid = 'ex_id_seq'::regclass AND refobjid = 'ex'::regclass AND deptype = 'a';

Gives the error:

user root does not have DELETE privilege on relation pg_depend

The bug is not related to the code, but just to the tests. @rafiss could we give a user this privilege ? I didn't see it skimming the doc... (show grants gives f for is_grantable on pg_depend...):

grant DELETE on pg_depend to root;
--> ERROR: invalid privilege type DELETE for virtual_table

On meta-testing. This took me longer than expected. I'm a bit unsure about the return on investment for simple tests, as manipulating AST can feel a bit tricky. I still pursued it because it seems that the gems (parser and unparser) are quite stable, and if we could use this not only in tests, that would be tremendous. We would need more guaranties though. And we'd need to verify the performances..

I was thinking about checking the hash of the method to be changed against a saved hash (which would be stored in an argument). If there is a change we warn. So:

CopyCat.copy_methods(..., hash: "somehash") do ... end
# => WARN: the method XXX was updated, here's the new changed method: ...
#      If it is ok change the hash to "somenewhash"

I think this is the way to go, but there are definitely some tweaks to make it easier.

BuonOmo avatar Jan 27 '24 21:01 BuonOmo

Somehow the test test_pk_and_sequence_for_returns_nil_if_no_pk works locally and not on CI...

BuonOmo avatar Mar 29 '24 19:03 BuonOmo

About test_pk_and_sequence_for_returns_nil_if_no_pk. The table creation returns a line:

rowid INT8 NOT VISIBLE NOT NULL DEFAULT unique_rowid()

This line is the one that is failing us...

BuonOmo avatar Jun 26 '24 10:06 BuonOmo

It should be good for merging now! I think I'll work on the transaction related issues next, and then we'll almost be done with those red tests.

BuonOmo avatar Jun 29 '24 10:06 BuonOmo