saltstack / saltstack/salt

[BUG] `file.serialize` does not merge lists even if `merge_if_exists: True` is set

Open
#66,903 2 comments 0 reactions 0 assignees View on GitHub

Nobody has claimed this yet.

bug needs-triage
Dominant language
Python
Stars
15.7k
Forks
5.6k
Avg merge
2d 44m
Merged PRs (30d)
80

Description

Description
I am using file.serialize to update the contents of a JSON file. Since that file may already exist, I have set merge_if_exists: True in order to merge the file contents with the dataset given in the state. Now if both the file and dataset have a list, Salt does not merge the lists but instead overwrites it with version from dataset.

Setup
(Please provide relevant configs and/or SLS files (be sure to remove sensitive info. There is no general set-up of Salt.)

Please be as specific as possible and give set-up details.

  • on-prem machine
  • VM (Virtualbox, KVM, etc. please specify)
  • VM running on a cloud service, please be explicit and add details
  • container (Kubernetes, Docker, containerd, etc. please specify)
  • or a combination, please be explicit
  • jails if it is FreeBSD
  • classic packaging
  • onedir packaging
  • used bootstrap to install

Steps to Reproduce the behavior
(Include debug logs if possible and relevant)

Create following 3 files:

9d2538af0da3:/ # cat /srv/salt/top.sls
base:
  '*':
    - bar

9d2538af0da3:/ # cat /srv/salt/bar.sls
write_test_file:
  file.serialize:
    - name: /test.json
    - dataset:
        eric: bacon
        john:
          - eggs
          - spam
    - serializer: json
    - merge_if_exists: True
    - show_changes: True

9d2538af0da3:/ # cat /test.json
{
  "terry": [
    "spam",
    "bacon"
  ],
  "john": [
    "beans"
  ]
}

Then apply the state or run test-mode:

9d2538af0da3:/ # salt-call --local state.apply bar test=True
local:
----------
          ID: write_test_file
    Function: file.serialize
        Name: /test.json
      Result: None
     Comment: Dataset will be serialized and stored into /test.json
     Started: 15:51:31.413810
    Duration: 17.361 ms
     Changes:
              ----------
              diff:
                  ----------
                  eric:
                      ----------
                      new:
                          bacon
                      old:
                          <_null_>
                  john:
                      ----------
                      new:
                          - eggs
                          - spam
                      old:
                          - beans

Summary for local
------------
Succeeded: 1 (unchanged=1, changed=1)
Failed:    0
------------
Total states run:     1
Total run time:  17.361 ms

Expected behavior

The lists from the file and from dataset should be merged, like this:

9d2538af0da3:/ # salt-call --local state.apply bar test=True
local:
----------
          ID: write_test_file
    Function: file.serialize
        Name: /test.json
      Result: None
     Comment: Dataset will be serialized and stored into /test.json
     Started: 15:53:02.186886
    Duration: 17.415 ms
     Changes:
              ----------
              diff:
                  ----------
                  eric:
                      ----------
                      new:
                          bacon
                      old:
                          <_null_>
                  john:
                      ----------
                      new:
                          - beans
                          - eggs
                          - spam
                      old:
                          - beans

Summary for local
------------
Succeeded: 1 (unchanged=1, changed=1)
Failed:    0
------------
Total states run:     1
Total run time:  17.415 ms

Versions Report

salt --versions-report (Provided by running salt --versions-report. Please also mention any differences in master/minion versions.)
9d2538af0da3:/ # salt-call --versions-report
Salt Version:
          Salt: 3006.0

Python Version:
        Python: 3.11.9 (main, Apr 08 2024, 06:18:15) [GCC]

Dependency Versions:
          cffi: 1.17.0
      cherrypy: Not Installed
   contextvars: 2.4
      dateutil: Not Installed
     docker-py: Not Installed
         gitdb: Not Installed
     gitpython: Not Installed
        Jinja2: 3.1.4
       libgit2: Not Installed
  looseversion: 1.3.0
      M2Crypto: 0.42.0
          Mako: Not Installed
       msgpack: 1.1.0
  msgpack-pure: Not Installed
  mysql-python: Not Installed
     packaging: 24.1
     pycparser: 2.22
      pycrypto: Not Installed
  pycryptodome: Not Installed
        pygit2: Not Installed
  python-gnupg: Not Installed
        PyYAML: 6.0.1
         PyZMQ: 25.1.2
        relenv: Not Installed
         smmap: Not Installed
       timelib: Not Installed
       Tornado: 4.5.3
           ZMQ: 4.3.5

System Versions:
          dist: opensuse-tumbleweed 20240917 n/a
        locale: utf-8
       machine: x86_64
       release: 6.10.10-arch1-1
        system: Linux
       version: openSUSE Tumbleweed 20240917 n/a

Additional context

The issue seems to be a missing third parameter merge_lists=True in this function call:

                merged_data = salt.utils.dictupdate.merge_recurse(
                    existing_data, dataset
                )

in https://github.com/saltstack/salt/blob/246d0664577ef72da8bd1f0c4dff0d18b4428b23/salt/states/file.py#L8470

Contributor guide

Open the contributing guide

First steps

  1. Read the whole issue, then the project's contributing guide.
  2. Comment on the issue to say you are picking it up — it saves two people doing the same work.
  3. Fork the repository and make your change on a branch.
  4. Open a pull request that references the issue number.

Research direction

Start in salt/states/file.py at the file.serialize merge_recurse call linked in the issue, then trace how merge_if_exists handles existing JSON data. Add regression coverage for lists under file.serialize and verify that the existing and dataset list values are merged as shown in the expected output.

Written by the indexing model from the issue text.

Assessment

Tech stack
python
Domain
infrastructure
Issue type
Bug
Difficulty
2/5
Estimated time
1-3 hours
Activity status
Stale
Clarity
Clearly specified
Newbie friendliness
52/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.