Fix/deal with truncated trigger names

[mindreframer]

Hey @cheerfulstoic ,

Thanks for EctoWatch! I hit the max trigger length bug in my app, it did cost me quite some hours to understand the reasons behind it.

My solution was to provide a checksum for very long labels, it seems to be working OK.
The PR is not backwards-compatible, so a proper version bump would be required.

I have looked at the other open PR, and I think this approach is more robust.

Cheers and I hope this helps.
Roman

[cheerfulstoic]

Hey, sorry that it took a while for you to figure out. It might take a bit for me to look at this, but know that it is on my list

[mindreframer]

@cheerfulstoic Hey, good to know! I'm not in hurry, right now I'm using my fork.

[cheerfulstoic]

Thanks for this. I'd actually really like to get this bug fixed, but I'd like to separate out the bug fixing from the refactoring and the breaking changes. I think some refactoring is worthwhile, for sure.

For now I'm going to release this PR as a bug fix because there aren't any breaking changes. Then I'll come back and think about the larger issues...

I was thinking about releasing a 1.0.0 since I haven't had any issues other this for a while. So maybe the breaking changes could come as part of that (but maybe it would be good to have a 0.13 out for a while before declaring 1.0 🤷 )

[cheerfulstoic]

Suggested change

	defmodule EctoWatchTest do
	defmodule EctoWatchLabelsTest do

[cheerfulstoic]

I think that this can just be:

Suggested change

	Map.new([
	{
	repo_mod,
	%{
	diff_triggers: MapSet.difference(found_triggers, specified_triggers),
	diff_functions: MapSet.difference(found_functions, specified_functions),
	specified_triggers: specified_triggers,
	specified_functions: specified_functions,
	found_triggers: found_triggers,
	found_functions: found_functions
	}
	}
	])
	%{
	repo_mod =>
	%{
	diff_triggers: MapSet.difference(found_triggers, specified_triggers),
	diff_functions: MapSet.difference(found_functions, specified_functions),
	specified_triggers: specified_triggers,
	specified_functions: specified_functions,
	found_triggers: found_triggers,
	found_functions: found_functions
	}
	}

[mindreframer]

I did not know, that you can just give variables as keys into hash map literals, cool!

[cheerfulstoic]

The maximum length isn't always 63. It's based on NAMEDATALEN - 1 and can also be fetched with SHOW max_identifier_length

It's probably not likely that these lengths will change, but I'd rather not deal with the support issue years down the line if it can be avoided 😅

[mindreframer]

Really? I guess we need to fetch it and cache once somewhere at the start

[cheerfulstoic]

This is nice, in a way, but it also loses information. While it's maybe a bit annoying, I think it's better to require the user to step in if the string is too long and require that the developer specify the label option. That way if somebody is debugging it will be easier for them to match what's in the database with what's in their code.

This is the approach taken for indexes / constraints with ecto_sql and so that's another good reason to go with it (both because it's a tested solution, and because people are familiar with the pattern)

[mindreframer]

Yeah, maybe that is better. I wanted something bulletproof without further input from the user. Hence this might be not such a bad approach, since there are usually only few very long tables, and their full names are not so hard to guess. Also the triggers are associated with a particular table, which give a clear hint about the exact name.

ALSO: we could add this as some meta info in the code, so that it's very quick to look up the full config (full label, short label, etc..)

[cheerfulstoic]

Any changes to the function / trigger names are a breaking change for users, and so will require a new minor release (I've been using the semver convention that when a library is pre-1.0, "minor" updates are more like "major" updates and "patch" updates are for both minor and patch changes). It's definitely doable and worth doing pre-1.0, but it's something that will need to be put into the documentation. Also developers would need to set the ECTO_WATCH_CLEANUP variable to remove the old functions / triggers.

So, in short, I'd like to get it really nailed down. Two open questions in my mind:

• Is the ew_ enough of a disambiguation to avoid accidentally deleting functions / triggers which are otherwise created (probably, but maybe it could be a _ew_ prefix or something...)
• Is it worth changing _func / _trig to _f / _t? Probably that's too much shortening for readability, but I'm just thinking about it and you're already shortening _deleted_ to _d_ here 🤔

I wrote above about requiring users to specify the label option, and I've honestly thought about requiring them to always specify label because it could make things simpler in some ways. We maybe wouldn't even need prefixes / suffixes... Anyway, trying to think of all these things before making a big breaking change.

[mindreframer]

Any changes to the function / trigger names are a breaking change for users, and so will require a new minor release (I've been using the semver convention that when a library is pre-1.0, "minor" updates are more like "major" updates and "patch" updates are for both minor and patch changes). It's definitely doable and worth doing pre-1.0, but it's something that will need to be put into the documentation. Also developers would need to set the ECTO_WATCH_CLEANUP variable to remove the old functions / triggers.

Absolutely. I'm quite sure, that it's not possible to fix this issue without a breaking change, hence I did not bother much to keep it nice. This means it would be required to drop all "old" triggers and install new ones. Maybe provide a dedicated module for this, like EctoWatch.Migrator.FixTruncatedLongNames.run(), that can be executed in controlled fashion in applications Ecto migrations.

• Is the ew_ enough of a disambiguation to avoid accidentally deleting functions / triggers which are otherwise created (probably, but maybe it could be a _ew_ prefix or something...)

Yeah, a special char in the beginning might help.

• Is it worth changing _func / _trig to _f / _t? Probably that's too much shortening for readability, but I'm just thinking about it and you're already shortening _deleted_ to _d_ here 🤔

Not sure, if that changes much.

I wrote above about requiring users to specify the label option, and I've honestly thought about requiring them to always specify label because it could make things simpler in some ways. We maybe wouldn't even need prefixes / suffixes... Anyway, trying to think of all these things before making a big breaking change.

That is a bigger topic. I currently do not have an opinion on this. Feel free to spike an alternative solution and see if that helps with cleaner code / less breakage / simpler handling.

[mindreframer]

I was thinking about releasing a 1.0.0 since I haven't had any issues other this for a while. So maybe the breaking changes could come as part of that (but maybe it would be good to have a 0.13 out for a while before declaring 1.0 🤷 )

I guess it would be better to go with 0.13 and see how stable the solution is. Collect feedback and once the dust settles, you could release 1.0.0.

From my side: I wont be working more on this PR, since my feature was shipped and right now I'm on a different project. This solution was good enough for me (obviously 😄), but I'm quite aware that there are shortcomings. Feel free to pick some ideas from this PR and implement them in a way that suites you better, my intention was to scratch my own itch and show you a possible solution. If this PR feels too invasive, close it. I mostly care about the final outcome, and don't need personal recognition or similar. It's just code that I had to fix in some way during crunch time, so it's far from perfect.

Either way, if you want to take some more time for a different solution, please add some emphasized hints in the Readme about this issue. I was going crazy trying to understand, why a single table was not triggering on changes.

Best,
Roman