Read Snowflake views definitions with the Snowflake-specific query

[LonwoLonwo]

Impact

• Bug fix (non-breaking change which fixes expected existing functionality)
• Enhancement/New feature (adds functionality without impacting existing logic)
• Breaking change (fix or feature that would cause existing functionality to change)

Description

Hello again! More enhancements for the Snowflake database, yes.

Now I would like to introduce you minor enhancement about the correct Snowflake view definitions reading.

Why did we do it? Because one of our users reported about NPE during the schema comparison in the Snowflake.
The error was here: https://github.com/liquibase/liquibase/blob/master/liquibase-core/src/main/java/liquibase/snapshot/jvm/ViewSnapshotGenerator.java#L86
A definition can be null in some side cases. AbstractJdbcDatabase#getViewDefinition can return null explicitly.
Unfortunately, we could not reproduce this error. And decided to improve reading definitions for the Snowflake database at least. To increase the chances of the fact that the NPE will not happen again.

Example for view:

CREATE OR REPLACE TABLE employees (title VARCHAR, employee_ID INTEGER, manager_ID INTEGER);

INSERT INTO employees (title, employee_ID, manager_ID) VALUES
    ('President', 1, NULL),  -- The President has no manager.
        ('Vice President Engineering', 10, 1),
            ('Programmer', 100, 10),
            ('QA Engineer', 101, 10),
        ('Vice President HR', 20, 1),
            ('Health Insurance Analyst', 200, 20);

CREATE VIEW employee_hierarchy (title, employee_ID, manager_ID, "MGR_EMP_ID (SHOULD BE SAME)", "MGR TITLE") AS (
   WITH RECURSIVE employee_hierarchy_cte (title, employee_ID, manager_ID, "MGR_EMP_ID (SHOULD BE SAME)", "MGR TITLE") AS (
      -- Start at the top of the hierarchy ...
      SELECT title, employee_ID, manager_ID, NULL AS "MGR_EMP_ID (SHOULD BE SAME)", 'President' AS "MGR TITLE"
        FROM employees
        WHERE title = 'President'
      UNION ALL
      -- ... and work our way down one level at a time.
      SELECT employees.title, 
             employees.employee_ID, 
             employees.manager_ID, 
             employee_hierarchy_cte.employee_id AS "MGR_EMP_ID (SHOULD BE SAME)", 
             employee_hierarchy_cte.title AS "MGR TITLE"
        FROM employees INNER JOIN employee_hierarchy_cte
       WHERE employee_hierarchy_cte.employee_ID = employees.manager_ID
   )
   SELECT * 
      FROM employee_hierarchy_cte
);

Things to worry about

CREATE_VIEW_AS_PATTERN has private status in the original class. Maybe getter will be better. But I just stole it.
And trimming of the ";" symbol looks not good, but this is what the definition from Snowflake looks like.

[nvoxland]

Thanks for the fix and the good PR description. Mostly the PR is:

1. Changing to a different query to get the view definition which should be better than in the generic information_schema query
2. Ensuring the trailing ; gets removed

Correct? The NPE is being fixed by the fact that your new query should not be returning null definitions anymore?

I'm wondering if a better structure to the changse are:

1. Instead of overriding getVewDefinition() so much, if you move your new query to a GetViewDefinitionGeneratorSnowflake class that query will get plugged into the exiting logic, just using a different query. Less duplication and also taking advantage of any bugfixes/feature-improvements in the base getVewDefinition method in the future.
2. If you need to remove the trailing ; you may still have to override getViewDefinition(), but only with a call to super.getViewDefinition() and removing a trailing ; if the value is not null.
3. Add a "if not null" check in ViewSnapshotGenerator, since that code needs to be handling null values coming back from getViewDefinition and is not. That is not just a problem with snowflake, and would be good to fix regardless.

Think those changes would make it better, @LonwoLonwo ?

[nvoxland]

Did you get a chance to see my comment above, @LonwoLonwo ?

[LonwoLonwo]

Oh, sorry, @nvoxland for the late response. I missed the answer. Thanks for the reviewing and tips.

Do I need to add any logging in case like this?:

if (definition == null || definition.isEmpty()) {
            return null;
}

[nvoxland]

Do I need to add any logging in case like this?:

I don't have a strong opinion one way or the other.

It probably wouldn't hurt to add debug-level logging there. It can help people troubleshoot what's going on, and that's always good. Just as long as it's not such a common scenario that it's not adding value for troubleshooting.

[LonwoLonwo]

Ok, logging added. I hope that the "info" level is enough.

[nvoxland]

Initial/Pre-Review Thoughts

Changes are looking good, thanks for the updates.

Questions I have:

• None

Potential risks:

• None

What could make the full review difficult:

• Nothing

[filipelautert]

More ❄️ improvements, thanks!

[XDelphiGrl]

@filipelautert and @LonwoLonwo, hello!

• Is it possible to add an integration test for this fix?
• Can you please give me a bit more description on how to reproduce the bug? Was the diff between an empty SnowflakeDB and one poulated with the objects in the example SQL you provided?

Also: nice to see you again, @LonwoLonwo! 👋🏻

Internal Functional Snowflake Test Suite Started Here --> [Snowflake functional test suite started here: https://github.com/liquibase/liquibase-pro-tests/actions/runs/4622799974.

[LonwoLonwo]

Nice to see you again @XDelphiGrl also

Unfortunately, we could not reproduce the issue of our customer. Perhaps there were problems with the presence of permissions from the user or broken data. Therefore, it turns out that I added an explicit view definition reading for Snowflake and the processing of potential NPE handling. I apologize that I could not help anymore.

[XDelphiGrl]

@LonwoLonwo, I guessed that would be your answer. Your PRs always have excellent repro steps! I understand (and have experienced) being unable to replicate a bug that happens only in a customer environment. I'd be interested in hearing if this fixes their problem.

I'm running our (internal) Snowflake functional test suite. This should at least tell us if there is something impacted by this change. I see some failures but I am 93% certain they are not due to this PR. We're having a week of strange build issues...

I hope you have a lovely day!

[XDelphiGrl]

This PR enhances the logic used by Liquibase to retrieve and generate Snowflake VIEW definitions. It is anticipated that this fix addresses the null pointer mentioned in the description.

• Full set of Snowflake functional tests passing.
• Test harness execution passing.
• No additional testing required.

APPROVED

Thank you, @LonwoLonwo, for your patience as I worked through our test failure snafu. Your contributions are valued. Plus, I like working with you!