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

[CT-562] Allow for opt in to context method failures #5167

Closed
emmyoop opened this issue Apr 26, 2022 · 4 comments
Closed

[CT-562] Allow for opt in to context method failures #5167

emmyoop opened this issue Apr 26, 2022 · 4 comments
Labels
tech_debt Behind-the-scenes changes, with little direct impact on end-user functionality

Comments

@emmyoop
Copy link
Member

emmyoop commented Apr 26, 2022

toyaml, fromyaml, tojson and fromjson in dbt/context/base.py currently pass back the default (or None) if the value passed in is not serializable/deserializable without ever failing.

try:
return yaml.safe_dump(data=value, sort_keys=sort_keys)
except (ValueError, yaml.YAMLError):
return default

It should be possible to get the error out for the case when the value is unexpectedly serializable/deserializable. The existing methods should continue to function as is to prevent breaking current projects. Add new methods that will throw errors.

    @contextmember
    @staticmethod
    def try_toyaml(
        value: Any, sort_keys: bool = False
    ) -> str:
        """The `tojson` context method can be used to serialize a Python
        object primitive, eg. a `dict` or `list` to a yaml string.  If the value 
        cannot be serialized it will raise an exception.
        :param value: The value serialize to yaml
        :param sort_keys: If True, sort the keys.
        Usage:
            {% set my_dict = {"abc": 123} %}
            {% set my_yaml_string = toyaml(my_dict) %}
            {% do log(my_yaml_string) %}
        """
        try:
            return yaml.safe_dump(data=value, sort_keys=sort_keys)
        except (ValueError, yaml.YAMLError):
            raise OurSpecialException()

Open Questions

  • What naming standard should we use for the new methods that will raise instead of default?
  • What exception should it raise and what information should be in that exception?
@emmyoop emmyoop added tech_debt Behind-the-scenes changes, with little direct impact on end-user functionality Team:Language labels Apr 26, 2022
@github-actions github-actions bot changed the title Allow for opt in to context method failures [CT-562] Allow for opt in to context method failures Apr 26, 2022
@jeremyyeo
Copy link
Contributor

Does a base ContextException make sense in this case? Doesn't appear as if we have such an exception.

@gshank
Copy link
Contributor

gshank commented May 3, 2022

I'm not convinced that ContextException is a great category. It's kind of getting into implementation details...

@jtcohen6
Copy link
Contributor

The existing methods should continue to function as is to prevent breaking current projects. Add new methods that will throw errors.

I had missed this piece several months ago.

As mentioned in #5107 (comment), as a person familiar with the try_cast / safe_cast SQL functions, my intuition would be that:

  • toyaml(value) will raise an exception if value cannot be converted to valid yaml
  • try_toyaml(value, default=None) will return default if value cannot be converted to valid yaml — i.e. the current behavior of the toyaml method

Some options to make that switch while also preserving backwards compatibility:

  • Deprecate the default argument of toyaml. (If users specify that argument, raise a deprecation warning and encourage them to switch to try_toyaml, since that's really what they're after.)
  • Deprecate toyaml, in favor of to_yaml(value) and try_to_yaml(value, default=None)
  • Come up with a different name, e.g. strict_toyaml, for the exception-raising version

@peterallenwebb
Copy link
Contributor

People don't remember what they were even worried about. Closing.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
tech_debt Behind-the-scenes changes, with little direct impact on end-user functionality
Projects
None yet
Development

No branches or pull requests

5 participants