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

A more flexible fix for custom tag constructors #279

Merged
merged 1 commit into from Mar 31, 2019

Conversation

@ingydotnet
Copy link
Member

@ingydotnet ingydotnet commented Mar 19, 2019

For #266 and #271. Replaces #273.

The yaml_loader class variable can be a list of loaders.
The YAMLObject helper class targets all the loader classes except SafeLoader.

Here's a test script that works. Uncomment the comments to support SafeLoader:

    #!/usr/bin/env python
    import yaml

    class Monster(yaml.YAMLObject):
        yaml_tag = u'!Monster'

        # yaml_loader = yaml.YAMLObject.yaml_loader
        # yaml_loader.append(yaml.SafeLoader)

        def __init__(self, name, hp, ac, attacks):
            self.name = name
            self.hp = hp
            self.ac = ac
            self.attacks = attacks
        def __repr__(self):
            return "%s(name=%r, hp=%r, ac=%r, attacks=%r)" % (
                self.__class__.__name__, self.name, self.hp, self.ac, self.attacks)

    input = """
    --- !Monster
    name: Cave spider
    hp: [2,6]    # 2d6
    ac: 16
    attacks: [BITE, HURT]
    """

    print(yaml.load(input))
    print(yaml.load(input, Loader=yaml.Loader))

    print(yaml.full_load(input))
    print(yaml.load(input, Loader=yaml.FullLoader))

    print(yaml.unsafe_load(input))
    print(yaml.load(input, Loader=yaml.UnsafeLoader))

    # print(yaml.safe_load(input))
    # print(yaml.load(input, Loader=yaml.SafeLoader))

Here is output:

$ PYTHONPATH=lib3 python3 test.py
test.py:27: YAMLLoadWarning: calling yaml.load() without Loader=... is deprecated, as the default Loader is unsafe. Please read https://msg.pyyaml.org/load for full details.
  print(yaml.load(input))
Monster(name='Cave spider', hp=[2, 6], ac=16, attacks=['BITE', 'HURT'])
Monster(name='Cave spider', hp=[2, 6], ac=16, attacks=['BITE', 'HURT'])
Monster(name='Cave spider', hp=[2, 6], ac=16, attacks=['BITE', 'HURT'])
Monster(name='Cave spider', hp=[2, 6], ac=16, attacks=['BITE', 'HURT'])
Monster(name='Cave spider', hp=[2, 6], ac=16, attacks=['BITE', 'HURT'])
Monster(name='Cave spider', hp=[2, 6], ac=16, attacks=['BITE', 'HURT'])

Works with Python 2 as well.

When someone writes a subclass of the YAMLObject class, the constructors
will now be added to all 3 (non-safe) loaders.

Furthermore, we support the class variable `yaml_loader` being a list,
offering more control of which loaders are affected.

To support safe_load in your custom class you could add this:

    yaml_loader = yaml.SafeLoader

    yaml_loader = yaml.YAMLObject.yaml_loader
    yaml_loader.append(yaml.SafeLoader)
@kgutwin
Copy link

@kgutwin kgutwin commented Mar 19, 2019

I like this solution. Were you also planning to address the convenience methods issue as raised in #271?

Also, while this isn't really in the scope for this PR, I noticed during my review that the convenience methods scan(), parse(), compose() and compose_all() still reference Loader rather than FullLoader as their default Loader. For consistency, it may make sense to update those to behave similarly to load() and load_all().

@ingydotnet
Copy link
Member Author

@ingydotnet ingydotnet commented Mar 19, 2019

@kgutwin Thanks for your time.

I think #274 is meant to address the convenience methods. You should review and comment there.

@perlpunk
Copy link
Member

@perlpunk perlpunk commented Mar 19, 2019

This could actually work. The only thing it would break is cls.yaml_loader in from_yaml(). But since from_yaml() gets a loader instance, probably nobody would use the class attribute.
For the record, just add this to the script:

    @classmethod
    def from_yaml(cls, loader, node):

        # do something with cls.yaml_loader

        data = loader.construct_mapping(node)
        print(data['name'])
        a, b = map(int, data['hp'].split('d'))
        return cls(data['name'], [a, b], data['ac'], data['attacks'])

and change the input like this:

input = """
--- !Monster
name: Cave spider
#hp: [2,6]    # 2d6
hp: 2d6
ac: 16
attacks: [BITE, HURT]
"""

The only thing I'm not sure about is if people would want to have their objects added to three loaders.

I would not add SafeLoader. Things should only be added to SafeLoader explicitly.

@perlpunk
Copy link
Member

@perlpunk perlpunk commented Mar 19, 2019

For #266 and #271.

I think it does not address #271

@perlpunk perlpunk added this to PRs and Notes to Consider in 5.2 Release Mar 19, 2019
@perlpunk
Copy link
Member

@perlpunk perlpunk commented Mar 19, 2019

To be really consistent, yaml.add_constructor should also add to three loaders.

@kgutwin
Copy link

@kgutwin kgutwin commented Mar 19, 2019

The only thing I'm not sure about is if people would want to have their objects added to three loaders.

If my experience is anything of a "typical user", I'd say the vast majority of people don't care about the internal structure of PyYAML Loaders and would just like the module to work 😄 That's why I think this fix is a good strategy - it solves the broadest set of cases, and for users who want to be more fine-grained about their use of Loaders, it's easy for them to set the yaml_loader class attribute.

@perlpunk
Copy link
Member

@perlpunk perlpunk commented Mar 22, 2019

I created #287 which adds the same kind of logic to yaml.add_constructor

@perlpunk perlpunk changed the base branch from master to release/5.2 Mar 31, 2019
@perlpunk perlpunk merged commit db91810 into release/5.2 Mar 31, 2019
4 checks passed
@perlpunk perlpunk moved this from PRs and Notes to Consider to PyYAML release/5.2 branch in 5.2 Release Mar 31, 2019
@perlpunk perlpunk deleted the ingydotnet/custom-constructor-fix branch Dec 2, 2019
netbsd-srcmastr pushed a commit to NetBSD/pkgsrc that referenced this issue Dec 15, 2019
5.2:
* Repair incompatibilities introduced with 5.1. The default Loader was changed,
  but several methods like add_constructor still used the old default
  yaml/pyyaml#279 -- A more flexible fix for custom tag constructors
  yaml/pyyaml#287 -- Change default loader for yaml.add_constructor
  yaml/pyyaml#305 -- Change default loader for add_implicit_resolver, add_path_resolver
* Make FullLoader safer by removing python/object/apply from the default FullLoader
  yaml/pyyaml#347 -- Move constructor for object/apply to UnsafeConstructor
* Fix bug introduced in 5.1 where quoting went wrong on systems with sys.maxunicode <= 0xffff
  yaml/pyyaml#276 -- Fix logic for quoting special characters
* Other PRs:
  yaml/pyyaml#280 -- Update CHANGES for 5.1
asherf added a commit to asherf/pants that referenced this issue Apr 28, 2020
https://github.com/yaml/pyyaml/blob/d0d660d035905d9c49fc0f8dafb579d2cc68c0c8/CHANGES#L7

5.3.1 (2020-03-18)

* yaml/pyyaml#386 -- Prevents arbitrary code execution during python/object/new constructor

5.3 (2020-01-06)

* yaml/pyyaml#290 -- Use `is` instead of equality for comparing with `None`
* yaml/pyyaml#270 -- fix typos and stylistic nit
* yaml/pyyaml#309 -- Fix up small typo
* yaml/pyyaml#161 -- Fix handling of __slots__
* yaml/pyyaml#358 -- Allow calling add_multi_constructor with None
* yaml/pyyaml#285 -- Add use of safe_load() function in README
* yaml/pyyaml#351 -- Fix reader for Unicode code points over 0xFFFF
* yaml/pyyaml#360 -- Enable certain unicode tests when maxunicode not > 0xffff
* yaml/pyyaml#359 -- Use full_load in yaml-highlight example
* yaml/pyyaml#244 -- Document that PyYAML is implemented with Cython
* yaml/pyyaml#329 -- Fix for Python 3.10
* yaml/pyyaml#310 -- increase size of index, line, and column fields
* yaml/pyyaml#260 -- remove some unused imports
* yaml/pyyaml#163 -- Create timezone-aware datetimes when parsed as such
* yaml/pyyaml#363 -- Add tests for timezone

5.2 (2019-12-02)
------------------

* Repair incompatibilities introduced with 5.1. The default Loader was changed,
  but several methods like add_constructor still used the old default
  yaml/pyyaml#279 -- A more flexible fix for custom tag constructors
  yaml/pyyaml#287 -- Change default loader for yaml.add_constructor
  yaml/pyyaml#305 -- Change default loader for add_implicit_resolver, add_path_resolver
* Make FullLoader safer by removing python/object/apply from the default FullLoader
  yaml/pyyaml#347 -- Move constructor for object/apply to UnsafeConstructor
* Fix bug introduced in 5.1 where quoting went wrong on systems with sys.maxunicode <= 0xffff
  yaml/pyyaml#276 -- Fix logic for quoting special characters
* Other PRs:
  yaml/pyyaml#280 -- Update CHANGES for 5.1
asherf added a commit to asherf/pants that referenced this issue Apr 29, 2020
https://github.com/yaml/pyyaml/blob/d0d660d035905d9c49fc0f8dafb579d2cc68c0c8/CHANGES#L7

5.3.1 (2020-03-18)

* yaml/pyyaml#386 -- Prevents arbitrary code execution during python/object/new constructor

5.3 (2020-01-06)

* yaml/pyyaml#290 -- Use `is` instead of equality for comparing with `None`
* yaml/pyyaml#270 -- fix typos and stylistic nit
* yaml/pyyaml#309 -- Fix up small typo
* yaml/pyyaml#161 -- Fix handling of __slots__
* yaml/pyyaml#358 -- Allow calling add_multi_constructor with None
* yaml/pyyaml#285 -- Add use of safe_load() function in README
* yaml/pyyaml#351 -- Fix reader for Unicode code points over 0xFFFF
* yaml/pyyaml#360 -- Enable certain unicode tests when maxunicode not > 0xffff
* yaml/pyyaml#359 -- Use full_load in yaml-highlight example
* yaml/pyyaml#244 -- Document that PyYAML is implemented with Cython
* yaml/pyyaml#329 -- Fix for Python 3.10
* yaml/pyyaml#310 -- increase size of index, line, and column fields
* yaml/pyyaml#260 -- remove some unused imports
* yaml/pyyaml#163 -- Create timezone-aware datetimes when parsed as such
* yaml/pyyaml#363 -- Add tests for timezone

5.2 (2019-12-02)
------------------

* Repair incompatibilities introduced with 5.1. The default Loader was changed,
  but several methods like add_constructor still used the old default
  yaml/pyyaml#279 -- A more flexible fix for custom tag constructors
  yaml/pyyaml#287 -- Change default loader for yaml.add_constructor
  yaml/pyyaml#305 -- Change default loader for add_implicit_resolver, add_path_resolver
* Make FullLoader safer by removing python/object/apply from the default FullLoader
  yaml/pyyaml#347 -- Move constructor for object/apply to UnsafeConstructor
* Fix bug introduced in 5.1 where quoting went wrong on systems with sys.maxunicode <= 0xffff
  yaml/pyyaml#276 -- Fix logic for quoting special characters
* Other PRs:
  yaml/pyyaml#280 -- Update CHANGES for 5.1
Eric-Arellano pushed a commit to pantsbuild/pants that referenced this issue May 1, 2020
https://github.com/yaml/pyyaml/blob/d0d660d035905d9c49fc0f8dafb579d2cc68c0c8/CHANGES#L7

5.3.1 (2020-03-18)

* yaml/pyyaml#386 -- Prevents arbitrary code execution during python/object/new constructor

5.3 (2020-01-06)

* yaml/pyyaml#290 -- Use `is` instead of equality for comparing with `None`
* yaml/pyyaml#270 -- fix typos and stylistic nit
* yaml/pyyaml#309 -- Fix up small typo
* yaml/pyyaml#161 -- Fix handling of __slots__
* yaml/pyyaml#358 -- Allow calling add_multi_constructor with None
* yaml/pyyaml#285 -- Add use of safe_load() function in README
* yaml/pyyaml#351 -- Fix reader for Unicode code points over 0xFFFF
* yaml/pyyaml#360 -- Enable certain unicode tests when maxunicode not > 0xffff
* yaml/pyyaml#359 -- Use full_load in yaml-highlight example
* yaml/pyyaml#244 -- Document that PyYAML is implemented with Cython
* yaml/pyyaml#329 -- Fix for Python 3.10
* yaml/pyyaml#310 -- increase size of index, line, and column fields
* yaml/pyyaml#260 -- remove some unused imports
* yaml/pyyaml#163 -- Create timezone-aware datetimes when parsed as such
* yaml/pyyaml#363 -- Add tests for timezone

5.2 (2019-12-02)
------------------

* Repair incompatibilities introduced with 5.1. The default Loader was changed,
  but several methods like add_constructor still used the old default
  yaml/pyyaml#279 -- A more flexible fix for custom tag constructors
  yaml/pyyaml#287 -- Change default loader for yaml.add_constructor
  yaml/pyyaml#305 -- Change default loader for add_implicit_resolver, add_path_resolver
* Make FullLoader safer by removing python/object/apply from the default FullLoader
  yaml/pyyaml#347 -- Move constructor for object/apply to UnsafeConstructor
* Fix bug introduced in 5.1 where quoting went wrong on systems with sys.maxunicode <= 0xffff
  yaml/pyyaml#276 -- Fix logic for quoting special characters
* Other PRs:
  yaml/pyyaml#280 -- Update CHANGES for 5.1
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
5.2 Release
PyYAML release/5.2 branch
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants