#1429 sync_schema does not sync triggers

[HansH]

In order to deal with #1429, I made two changes:

1. Instead of using CREATE TRIGGER IF NOT EXISTS..., it now uses DROP TRIGGER IF EXISTS <triggername>;CREATE TRIGGER <triggername>..... This ensures that the trigger is recreated as specified in the make_storage call.
2. I added a method trigger_names, similar to the method table_names. This can be used to see which triggers actually exist in the database, so you can decide e.g. to use drop_trigger() to delete any triggers that you do not want.

I also added tests to test these changes.

[fnc12]

hi @HansH . Thanks for creating this PR. I got one concern about it: if we are going to drop trigger anyway in sync_schema then this PR cannot be accepted cause it is not how sync intended to work. If you need this behavior for your specific domain/project please use storage.drop_trigger instead and them storage.sync_schema

[HansH]

If this is not how sync is intended to work, can you expand on how it is intended to work? Note that it immediately recreates the triggers that it drops. So in effect nothing is dropped, only changed, which I would expect to be the intended behavior of sync.

[fnc12]

If this is not how sync is intended to work, can you expand on how it is intended to work?

it has to compare real triggers from db with ones specified in make_storage and make db's ones to be the same as described in make_storage. Just like it works with tables. Imagine if every sync_schema drops all tables EVERY time. I know that triggers do not contain data but anyway we don't have to remove entities if we can skip removal.

Note that it immediately recreates the triggers that it drops.

I see. But it does drops even when no difference found. Table sync works differently.

So in effect nothing is dropped, only changed, which I would expect to be the intended behavior of sync.

It is ok for you personally and your project. But it may be not ok for everyone who uses sqlite_orm. That's why it couldn't be merged into sqlite_orm. But it can be stored in your fork and you can use your fork instead. Or just use drop_trigger as I said before. Of let's find a way how to compare trigger's state and drop it only when it really differs

[HansH]

Thanks for your response. I'm just trying to understand your reasoning, so I can see if I can change this PR to deal with your concerns. I'm still not entirely convinced that dropping the trigger would pose any problems, exactly because of the reason you mention: they don't contain data that will be lost. But for our use case it is not important. I was just hoping to make a contribution that may be valuable to other users of this project. But if you think this is not a good approach, that is fine with me too.

There is also another change in this pull request: adding the trigger_names method, that lists all the triggers that exist in the database (so not just the ones that are in the make_storage call). (b4322a6). Is that change acceptable to you? In that case I can make a new pull request for just that commit.

For us the latter would me much more useful than the dropping and recreation of triggers. Than we can just drop all triggers before calling sync_schema.

[fnc12]

Thanks for your response. I'm just trying to understand your reasoning, so I can see if I can change this PR to deal with your concerns. I'm still not entirely convinced that dropping the trigger would pose any problems, exactly because of the reason you mention: they don't contain data that will be lost. But for our use case it is not important. I was just hoping to make a contribution that may be valuable to other users of this project. But if you think this is not a good approach, that is fine with me too.

Imagine the code dropped trigger but before creating exception occurred not matter the reason. So we got to the case when do did not have any reason to delete/recreate triggers but we did it and now DB doesn't have the trigger. It is not right. If there is no reason to delete anything we don't have to delete anything.

There is also another change in this pull request: adding the trigger_names method, that lists all the triggers that exist in the database (so not just the ones that are in the make_storage call). (b4322a6). Is that change acceptable to you? In that case I can make a new pull request for just that commit.

Yes it is a good addition. Let's make this or another PR with only this feature and we'll review it. Thanks

[fnc12]

I got an idea how to sync triggers. It is not ideal but it may work and doesn't produce unused deletions of triggers: we extract trigger's SQL string from sqlite_master, we serialize trigger object and recreating trigger if strings are not equal. It may product one non necessary deletion only once if trigger existed before, has the same structure but differs in something not important like one more space or symbols case differs. But it is not a big deal I think. @trueqbit what do you think?

[trueqbit]

I got an idea how to sync triggers. It is not ideal but it may work and doesn't produce unused deletions of triggers: we extract trigger's SQL string from sqlite_master, we serialize trigger object and recreating trigger if strings are not equal. It may product one non necessary deletion only once if trigger existed before, has the same structure but differs in something not important like one more space or symbols case differs. But it is not a big deal I think. @trueqbit what do you think?

The serialized version of the trigger in sqlite_schema is the single source of truth (in absence of any other type-based master table), so I agree that this is definitely a good approach.

[fnc12]

@HansH please fix formatting and merge conflicts

[HansH]

With #1434, this issue has been sufficiently solved for me. I don't think it's worthwhile to continue with this pull request. I do see you have ideas for solving the original problem. But that is a whole different approach, and I don't think it makes sense to do that as part of this pull request.