feat: add decorator to guard public APIs

[betodealmeida]

EDIT

Instead of the decorator approach described below I implemented the guard as a custom unit test.

SUMMARY

This PR adds a decorator called guard, used to protect public APIs from breaking changes without a major version bump (see #12566). If the API is changed the user receives a warning.

For example, we allow people to specify a custom function called SQL_QUERY_MUTATOR in their superset_config.py. This API is already broken, since the signature described in superset/config.py is out-of-sync with how it's actually called:

# superset/config.py
# A function that intercepts the SQL to be executed and can alter it.
# The use case is can be around adding some sort of comment header
# with information such as the username and worker node information
#
#    def SQL_QUERY_MUTATOR(sql, username, security_manager):
#        dttm = datetime.now().isoformat()
#        return f"-- [SQL LAB] {username} {dttm}\n{sql}"
SQL_QUERY_MUTATOR = None

If we look at where the function is called we can see that the signature should be slightly different:

# superset/sql_lab.py
SQL_QUERY_MUTATOR = config["SQL_QUERY_MUTATOR"]
...
if SQL_QUERY_MUTATOR:
        sql = SQL_QUERY_MUTATOR(sql, user_name, security_manager, database)

Notice the extra database argument, and the small difference between username and user_name. This was probably changed at some point without updating the config, breaking any existing SQL_QUERY_MUTATOR that don't expect the database argument!

To prevent this in the future, we create a dummy function and protect it:

@guard("W}V8JTUVzx4ur~{CEhT?")
def dummy_sql_query_mutator(
    sql: str,
    user_name: str,
    security_manager: SupersetSecurityManager,
    database: Database,
) -> str:
    """A no-op version of SQL_QUERY_MUTATOR"""
    return sql

We change the code a little bit so that the function is always called (either the custom or the dummy one):

SQL_QUERY_MUTATOR = config.get("SQL_QUERY_MUTATOR") or dummy_sql_query_mutator
...
sql = SQL_QUERY_MUTATOR(sql, user_name, security_manager, database)

Now what happens if we change the signature of the function? Let's try it:

@guard("W}V8JTUVzx4ur~{CEhT?")
def dummy_sql_query_mutator(
    sql: str,
    user_name: str,
    security_manager: SupersetSecurityManager,
    database: Database,
    foo: int,
) -> str:
    """A no-op version of SQL_QUERY_MUTATOR"""
    return sql

Changing the code like this will print the following warning:

/Users/beto/Projects/incubator-superset/superset/utils/decorators.py:125: UserWarning: The decorated object
`dummy_sql_query_mutator` (in /Users/beto/Projects/incubator-superset/superset/sql_lab.py line 54) has a
public interface which has currently been modified. This MUST only be released in a new major version of
Superset according to SIP-57. To remove this warning message update the hash in the `guard` decorator to
';>K{8wd)&8!=9?1qR@Ja'.

    @guard("W}V8JTUVzx4ur~{CEhT?")
    def dummy_sql_query_mutator(
        sql: str,
        user_name: str,
        security_manager: SupersetSecurityManager,
        database: Database,
        foo: int,
    ) -> str:
        """A no-op version of SQL_QUERY_MUTATOR"""
        return sql

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

N/A

TEST PLAN

Changed signature and run unit tests, it printed the warning message above.

ADDITIONAL INFORMATION

• Has associated issue:
• Changes UI
• Requires DB Migration.
• Confirm DB Migration upgrade and downgrade tested.
• Introduces new feature or API
• Removes existing feature or API

[ktmud]

I know hashing is fast for most cases, but I'm wondering whether we can move this to be an offline job in some way?

I.e. instead of a decorator, store the hash for all public APIs in a separate file and run the checks in CI.

[betodealmeida]

I know hashing is fast for most cases, but I'm wondering whether we can move this to be an offline job in some way?

I.e. instead of a decorator, store the hash for all public APIs in a separate file and run the checks in CI.

Good point... we could also just add unit tests that import the functions and compare their signature hashes to hard coded values, that might be a more efficient approach.

(Also, the hashes are computed just once when the files are "compiled", not when the functions are called.)

[codecov-io]

Codecov Report

Merging #12635 (beef427) into master (e7def7e) will decrease coverage by 3.44%.
The diff coverage is 82.35%.

@@            Coverage Diff             @@
##           master   #12635      +/-   ##
==========================================
- Coverage   66.80%   63.35%   -3.45%     
==========================================
  Files        1017      487     -530     
  Lines       49730    30014   -19716     
  Branches     4864        0    -4864     
==========================================
- Hits        33221    19016   -14205     
+ Misses      16386    10998    -5388     
+ Partials      123        0     -123     

Flag	Coverage Δ
cypress	?
javascript	?
python	63.35% <82.35%> (-0.63%)	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
superset/config.py	90.61% <ø> (ø)
superset/utils/public_interfaces.py	78.57% <78.57%> (ø)
superset/sql_lab.py	80.18% <100.00%> (+0.74%)	⬆️
superset/db_engines/hive.py	0.00% <0.00%> (-85.72%)	⬇️
superset/sql_validators/postgres.py	50.00% <0.00%> (-50.00%)	⬇️
superset/db_engine_specs/hive.py	54.61% <0.00%> (-29.24%)	⬇️
superset/databases/commands/create.py	83.67% <0.00%> (-8.17%)	⬇️
superset/databases/commands/update.py	85.71% <0.00%> (-8.17%)	⬇️
superset/connectors/sqla/models.py	84.31% <0.00%> (-6.28%)	⬇️
superset/databases/commands/test_connection.py	84.78% <0.00%> (-4.35%)	⬇️
... and 549 more

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e7def7e...beef427. Read the comment docs.

[betodealmeida]

I reviewed superset/config.py today with @eschutho, and it looks like we need to guard a good number of objects:

• STATS_LOGGER
• EVENT_LOGGER
• CUSTOM_SECURITY_MANAGER
• SQLALCHEMY_CUSTOM_PASSWORD_STORE
• GET_FEATURE_FLAGS_FUNC
• LOGGING_CONFIGURATOR
• SQLLAB_CTAS_SCHEMA_NAME_FUNC
• CSV_TO_HIVE_UPLOAD_DIRECTORY_FUNC
• ALLOWED_USER_CSV_SCHEMA_FUNC
• FLASK_APP_MUTATOR
• TRACKING_URL_TRANSFORMER
• DB_CONNECTION_MUTATOR
• SQL_QUERY_MUTATOR
• WEBDRIVER_AUTH_FUNC
• SQLA_TABLE_MUTATOR
• QUERY_COST_FORMATTERS_BY_ENGINE

[betodealmeida]

@ktmud this is what I had in mind; now we can just update hashes and each one will get its own unit test.

[ktmud]

Nice. Not that it matters much, but can pytest display which parameter failed the test in case of assertion errors?

[betodealmeida]

It does, it treats each parameter as a separate test called unit_test_name[parameters]: https://docs.pytest.org/en/stable/parametrize.html#pytest-mark-parametrize

[ktmud]

Should we clean up the decorator?

[betodealmeida]

Should we clean up the decorator?

We should. I'm emotionally attached to it, but I'll remove it. :-P