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

3000 release annouced the deprecation of json_decode_dict is not equalivant as tojson #55911

Closed
sjorge opened this issue Jan 18, 2020 · 6 comments
Assignees
Labels
expected-behavior intended functionality
Milestone

Comments

@sjorge
Copy link
Contributor

sjorge commented Jan 18, 2020

Description of Issue

With the 3000rc dropping the deprecation of json_decode_dict in the next release was announced. The new way to do it is to use tojson.

In prepare my states and pillars before upgrading from 2018.2.3 to 3000rc2 I noticed issues when replacing json_decode_dict with tojson.

json_decode_dict would output an in the order it was received, tojson will output the keys sorted alphabetically!

Setup

{% load_yaml as x %}
z: some
y: text
x: here
{% endload %}
/tmp/tojson.out:
  file.managed:
    - contents: |
        {{ x|tojson }}

/tmp/json_decode_dict.out:
  file.managed:
    - contents: |
        {{ x|json_decode_dict }}

Steps to Reproduce Issue

I provided a basic test state to see it in action, notice the different order between tosjon and json_decode_dict

Versions Report

Salt Version:
           Salt: 2019.2.3

Dependency Versions:
           cffi: 1.11.5
       cherrypy: 17.3.0
       dateutil: 2.7.3
      docker-py: Not Installed
          gitdb: 2.0.5
      gitpython: 2.1.11
          ioflo: Not Installed
         Jinja2: 2.10
        libgit2: Not Installed
        libnacl: 1.6.1
       M2Crypto: 0.30.1
           Mako: Not Installed
   msgpack-pure: Not Installed
 msgpack-python: 0.5.6
   mysql-python: Not Installed
      pycparser: 2.19
       pycrypto: 3.6.6
   pycryptodome: Not Installed
         pygit2: Not Installed
         Python: 2.7.15 (default, Oct 23 2018, 17:11:49)
   python-gnupg: 0.4.3
         PyYAML: 3.13
          PyZMQ: 17.1.2
           RAET: Not Installed
          smmap: 0.9.0
        timelib: Not Installed
        Tornado: 5.1.1
            ZMQ: 4.2.5

System Versions:
           dist:
         locale: UTF-8
        machine: i86pc
        release: 5.11
         system: SunOS
        version: Not Installed
@xeacott
Copy link
Contributor

xeacott commented Jan 21, 2020

I was looking at the documentation and it doesn't appear to have anything indicating that the output between those 2 flags are different, just that tojson is suppose to replace it. Is having them sorted unwanted behavior?

Here's the docs page for reference too... https://jinja.palletsprojects.com/en/2.10.x/templates/#tojson

@xeacott xeacott added severity-low 4th level, cosemtic problems, work around exists pending-changes The pull request needs additional changes before it can be merged labels Jan 21, 2020
@xeacott xeacott added this to the Approved milestone Jan 21, 2020
@sjorge
Copy link
Contributor Author

sjorge commented Jan 21, 2020

Depending on what output we're trying to cast, order might be imporant. For me a lot of things I had in a specific order got sorted alphabetically. e.g. some share templates showed up after the share that used them, resulting in errors in the smb.conf

@xeacott
Copy link
Contributor

xeacott commented Jan 21, 2020

Understood, so it seems quite undesirable especially when it isn't clear to the user. Thanks for the report!

@xeacott xeacott added Bug broken, incorrect, or confusing behavior severity-medium 3rd level, incorrect or bad functionality, confusing and lacks a work around P3 Priority 3 and removed severity-low 4th level, cosemtic problems, work around exists pending-changes The pull request needs additional changes before it can be merged labels Jan 21, 2020
@sagetherage sagetherage added ZRELEASED - Neon retired label severity-critical top severity, seen by most users, serious issues labels Jan 22, 2020
@waynew
Copy link
Contributor

waynew commented Jan 24, 2020

@sjorge json_decode_dict is actually an incorrectly named function, which is why it was deprecated in 2018.3.0.

To retain the same functionality, use json_encode_dict:

{% load_yaml as x %}
z: some
y: text
x: here
{% endload %}
/tmp/tojson.out:
  file.managed:
    - contents: |
        {{ x|json_encode_dict }}     # Note `en`code

/tmp/json_decode_dict.out:
  file.managed:
    - contents: |
        {{ x|json_decode_dict }}

The tojson filter provides a different functionality, and actually comes from the Jinja library.

@sjorge
Copy link
Contributor Author

sjorge commented Jan 24, 2020

@waynew should the release ntoes be updated then? It says the json_decode_dict is deprecated and to use tojson instead... ?

Edit: yep using json_encode_dict gets the correct behavior again.

@sagetherage sagetherage removed the ZRELEASED - Neon retired label label Jan 24, 2020
@waynew
Copy link
Contributor

waynew commented Jan 28, 2020

If one wanted the correct behavior of a function called json_decode_dict, tojson is the right filter. What json_decode_dict did originally is not json_decode_dict, it was doing json_encode_dict.

Basically you could think of it as being called "undo" when it should have been "do".

And for most cases, people were looking for "do", and tojson is what they're wanting.

I'm going to go ahead and close this issue, but reproducing it has exposed a potential issue when migrating from 2 to 3.

@waynew waynew added expected-behavior intended functionality and removed Bug broken, incorrect, or confusing behavior severity-medium 3rd level, incorrect or bad functionality, confusing and lacks a work around P3 Priority 3 severity-critical top severity, seen by most users, serious issues labels Jan 28, 2020
@waynew waynew closed this as completed Jan 28, 2020
waynew added a commit to waynew/salt that referenced this issue Mar 3, 2021
Ch3LL pushed a commit that referenced this issue Mar 5, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
expected-behavior intended functionality
Projects
None yet
Development

No branches or pull requests

4 participants