Add missing parameters to Avram::Database.truncate method

[akadusei]

Missed these in #980

[jwoertink]

oops! 😅 Guess I missed these too... I thought we had specs around truncate, but maybe not? Would you be able to add a spec in just so we have these accounted for in case there's other changes?

[akadusei]

I thought we had specs around truncate, but maybe not?

avram/spec/avram/queryable_spec.cr

Lines 1244 to 1266 in 0b9797b

	describe ".truncate" do
	it "truncates the table" do
	10.times { UserFactory.create }
	UserQuery.new.select_count.should eq 10
	# NOTE: we don't test rows_affected here because this isn't
	# available with a truncate statement
	UserQuery.truncate
	UserQuery.new.select_count.should eq 0
	end
	it "deletes associated data when cascade is true" do
	post_with_matching_comment = PostFactory.create
	comment = CommentFactory.new
	.post_id(post_with_matching_comment.id)
	.create
	PostQuery.truncate cascade: true
	PostQuery.new.select_count.should eq 0
	expect_raises(Avram::RecordNotFoundError) do
	CommentQuery.new.find(comment.id)
	end
	end
	end

There's none for Avram::Database.truncate, but I guess it's implicitly tested given that every test tagged with Avram::SpecHelper::TRUNCATE performs a truncate?

[akadusei]

Line 13 here produces same error in CockroachDB:

avram/src/avram/spec_helper.cr

Lines 10 to 15 in 0b9797b

	def self.wrap_spec_in_transaction(spec : Spec::Example::Procsy, *databases)
	if use_truncation?(spec)
	spec.run
	databases.each(&.truncate)
	return
	end

Should the call to #truncate factor in restart_identity and cascade arguments? If yes, what is the best way to do it?

[jwoertink]

That probably doesn't matter much for the specs, I would assume. Does Cockroach require this to get changed in order to work?

[akadusei]

Does Cockroach require this to get changed in order to work?

Yes. Avram::Database::DatabaseCleaner#truncate has restart_identity set to true by default, which inserts RESTART IDENTITY into the SQL statement. RESTART IDENTITY is not supported by Cockroach DB.

[jwoertink]

Gotcha... hmm 🤔 Would it be weird to have a property on the database that decides if it should have this or not? Maybe it's time to think about an engine property that's defaulted to postgresql, but has cockraochdb as an option.. Maybe this is an enum property or something. Then that line starts to look like

databases.each do |db|
    db.truncate(restart_identity: db.engine.postgresql?)
end

Or does that sound like too much for this case? Maybe a simple solution is just passing in that option? I'm not sure if there would be a time where you use postgres and don't want the restart_identity, but there probably is a case for that...

[akadusei]

I'm thinking more along the lines of checking for "no_restart_identity" tag on the spec, and setting restart_identity: false if found. (Or checking for "restart_identity" tag and setting restart_identity: true if found, depending on what we want as default)

Maybe it's time to think about an engine property that's defaulted to postgresql, but has cockraochdb as an option...

If we do this, we should not need to set restart_identity to get around the error. In the #truncate mehtods, we can simply check the db engine type and exclude "RESTART IDENTITY" from the resulting SQL statement. So the restart_identity parameter would still exist, but would do nothing in the case of Cockroach DB.

I believe this raises the question of the broader support for other DB engines as discussed in #924 (comment).

[jwoertink]

Ok. Maybe we just add the tag in as a temporary workaround for now with a TODO note about updating it later. I'm not sure the best direction to go.

I believe this raises the question of the broader support for other DB engines

Yeah, I still am of the feeling that I would love to avoid all the extra work of supporting extra engines. It's a bit frustrating that Cockroach says it's postgres compatible, but seems to have an asterisk. The tough part here is that we currently have no way to test if a change will break Cockroach. But I guess we're at the point that we either need to say we can't support it at all, or bite the bullet and just start supporting more things.

So I guess what I'm saying is, let's just hack around this issue for now so you're not blocked on anything, then we can start planning out a path to add more support for other engines as well as figuring out a way to test this to prevent this issue down the road.

[akadusei]

Ok. Maybe we just add the tag in as a temporary workaround for now with a TODO note about updating it later.

Sure, will see what I can come up with.

Yeah, I still am of the feeling that I would love to avoid all the extra work of supporting extra engines.

Me too. I figure a shard that extends Avram (mostly via monkey-patching), purposely for Cockroach DB (or whatever engine anyone needs supported) should do the trick. These could be maintained by persons outside the core team.

I added something similar in Lucille (GrottoPress/lucille@12cb929), but removed it after the relevant issues were fixed in #980. The idea is that any issues with any of the DBs would get patched in these shards (which should have shorter release cycles), and may or may not be patched upstream. In any case, for as long as an application does require "lucille/cockroach", for instance, it should be guaranteed to work with Cockroach.

The tough part here is that we currently have no way to test if a change will break Cockroach

I have added Lucky/Avram main branches to the tests in CI for all my shards, so any changes that break Cockroach DB should be caught (and hopefully fixed) before subsequent releases. This is probably not the ideal solution, but an extension shard (as before described) could add its own tests relevant for whatever DB it is supporting.

It's a bit frustrating that Cockroach says it's postgres compatible, but seems to have an asterisk.

From what I understand from their end, some things are difficult to implement because of the very nature of Cockroach's architecture. The few issues in Avram had to do with spec and migration helpers. It works very well otherwise; one could always drop down to raw SQL if desired.

So I guess what I'm saying is, let's just hack around this issue for now so you're not blocked on anything...

Don't worry about me 😃. I tend to find my way somehow. Whatever we decide should work well for the community at large.

[akadusei]

Calling #truncate directly inside the spec works even when using transactional specs, so I'm wondering if the whole .use_truncation? check should be deprecated/removed instead? That way no one has to worry about how #truncate is called inside .wrap_spec_in_transaction? 🤔

[jwoertink]

Yeah, you can do your own manual truncate in the specs, but it's currently setup to do it automatically for you so you don't have to call it on every spec. This was added in as part of a way to make specs run faster. I think if we took it out, then you'd have to decide how to clear your data between specs always whether that's with truncate or with transactional rollbacks. That may complicate things more, I think.

[jwoertink]

I think this is good, so I'm just gonna merge, but if there's more updates we need for this, let me know. Thanks @akadusei!