Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

accepted_values doesn't work properly when some of the values has the <'> (single quote) special character #10654

Closed
2 tasks done
nicods-fr opened this issue Sep 3, 2024 · 5 comments
Labels
bug Something isn't working dbt tests Issues related to built-in dbt testing functionality wontfix Not a bug or out of scope for dbt-core

Comments

@nicods-fr
Copy link

nicods-fr commented Sep 3, 2024

Is this a new bug in dbt-core?

  • I believe this is a new bug in dbt-core
  • I have searched the existing issues, and I could not find an existing issue for this bug

Current Behavior

accepted_values doesn't work properly when some of the values has the <'> (single quote) special character

Expected Behavior

SELECT 'Valeur avec l''apostrophe' AS col_avec_bug

This test doesn't work, but it should:

- models:
-   name: model_buggy
    columns:
      - name: col_avec_bug
        tests:
          - accepted_values:
            values:
              - "Valeur avec l'apostrophe"
              - "Autre valeur enum"

Steps To Reproduce

Same as expected behavior

Relevant log output

No response

Environment

- OS: All
- Python: 3.10
- dbt: 1.7.8 (I will test 1.8 later today)

Which database adapter are you using with dbt?

postgres

Additional Context

No response

@nicods-fr nicods-fr added bug Something isn't working triage labels Sep 3, 2024
@nicods-fr nicods-fr changed the title [Bug] <title>accepted_values doesn't work properly when some of the values has the <'> (single quote) special character accepted_values doesn't work properly when some of the values has the <'> (single quote) special character [Bug] Sep 3, 2024
@nicods-fr nicods-fr changed the title accepted_values doesn't work properly when some of the values has the <'> (single quote) special character [Bug] accepted_values doesn't work properly when some of the values has the <'> (single quote) special character Sep 3, 2024
@dbeatty10 dbeatty10 added the dbt tests Issues related to built-in dbt testing functionality label Sep 3, 2024
@dbeatty10
Copy link
Contributor

Thanks for reporting this @nicods-fr !

Easiest workaround

The easiest way to solve this: just use the syntax of your database to escape the quotes. In your particular case with Postgres:

models:
  - name: my_model
    columns:
      - name: status
        tests:
          - accepted_values:
              values:
                - "will"
                - "won''t"

Possible solution

A possible way to solve this (but it looks like it would break anyone using the workaround above) would be to use the escape_single_quotes macro here like:

        {{ dbt.string_literal(dbt.escape_single_quotes(value)) }}

Full version of accepted_values test with that modification (click to toggle visibility)

{% macro default__test_accepted_values(model, column_name, values, quote=True) %}

with all_values as (

    select
        {{ column_name }} as value_field,
        count(*) as n_records

    from {{ model }}
    group by {{ column_name }}

)

select *
from all_values
where value_field not in (
    {% for value in values -%}
        {% if quote -%}
        {{ dbt.string_literal(dbt.escape_single_quotes(value)) }}
        {%- else -%}
        {{ value }}
        {%- endif -%}
        {%- if not loop.last -%},{%- endif %}
    {%- endfor %}
)

{% endmacro %}

I tried this out, and it worked for me with the following YAML:

          - accepted_values:
              values:
                - will
                - won't

@dbeatty10
Copy link
Contributor

Operating under the belief that contractions within accepted_values are extremely uncommon and there exists an easy workaround, I'm going to close this as "not planned".

But if we get a lot of feedback that this is high impact, we can take another look.

@dbeatty10 dbeatty10 closed this as not planned Won't fix, can't repro, duplicate, stale Sep 3, 2024
@dbeatty10 dbeatty10 added wontfix Not a bug or out of scope for dbt-core and removed triage labels Sep 3, 2024
@nicods-fr
Copy link
Author

nicods-fr commented Sep 5, 2024

I did fix it with a workaround, overriding the bugged test. I could fix it and start contributing if I get the time to read the contribution guide

{#-/* Overriden DBT test */-#}

{%- macro test_accepted_values(model, column_name, values) -%}

{% if values is none or values|length == 0 %}
    {{ exceptions.raise_compiler_error("values in accepted_values test cannot be Null or empty") }}
{% endif %}

{%- for val in values %}
    {% if val is none %}
        {{ exceptions.raise_compiler_error("Null is not a valid value in accepted_values.values") }}
    {% endif %}
{%- endfor %}

WITH tested_values AS (
    {%- for v_ in values %}
        {% if not loop.first %} UNION ALL {% endif %}
        SELECT '{{ v_.replace("'", "''") }}' AS expected_val             {#/*"*/#}
    {%- endfor %}
),

vals_in_col AS (
    SELECT DISTINCT "{{column_name}}" FROM {{ model  }}
),
sorted_vals_in_col AS (
    SELECT "{{column_name}}"::TEXT AS actual_val FROM vals_in_col ORDER BY "{{column_name}}"
)

SELECT reality.actual_val AS bad_values

FROM sorted_vals_in_col reality
FULL JOIN tested_values expectation
    ON reality.actual_val = expectation.expected_val
WHERE expectation.expected_val IS NULL AND reality.actual_val IS NOT NULL

{%- endmacro %}

@nicods-fr
Copy link
Author

In my point of view, the proposed workaround is a bug. So anyone using it, should stop and fix their code. Also, the macro dbt.string_literal should escape single quotes, so doubling like this : dbt.string_literal(dbt.escape_single_quotes(value)) should produce too many quotes, if it doesn't, one of those macros has a bug.

@dbeatty10
Copy link
Contributor

Thanks for providing the details of your workaround @nicods-fr 🤩

Also, the macro dbt.string_literal should escape single quotes, so doubling like this : dbt.string_literal(dbt.escape_single_quotes(value)) should produce too many quotes, if it doesn't, one of those macros has a bug.

This is valid feedback! These macros originally came from the dbt_utils package, and they were ported over as-is. Since they've been around for a long time and many folks may be using them with full knowledge of their current implementations, we'd be unlikely to make that change to avoid breaking anyone.

I buy into the point you are making, and opened dbt-labs/dbt-adapters#293 as a result.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working dbt tests Issues related to built-in dbt testing functionality wontfix Not a bug or out of scope for dbt-core
Projects
None yet
Development

No branches or pull requests

2 participants