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

state file gets confused #115

Open
jaymzh opened this issue Jul 22, 2019 · 5 comments · Fixed by #116
Open

state file gets confused #115

jaymzh opened this issue Jul 22, 2019 · 5 comments · Fixed by #116

Comments

@jaymzh
Copy link
Collaborator

jaymzh commented Jul 22, 2019

the state file gets the same key in it twice, sometimes;

{"port":5257,"ssl":false,"ssh":true,"logging":true,"ref":null,"ref":"426f245cc96aeaf4ef2aa8c274294db91566417c"}

Note 'ref' is there twice... in my case I think this is one of 2 problems causing tt to restart itself all the time.

jaymzh added a commit to jaymzh/taste-tester that referenced this issue Jul 22, 2019
Moving data between JSON and hashes means that you can end up with :foo
and "foo" both in the same hash, which is bad. Use a mash so they are
always the same.

Closes facebook#115.

Signed-off-by: Phil Dibowitz <phil@ipom.com>
jaymzh added a commit to jaymzh/taste-tester that referenced this issue Jul 22, 2019
Moving data between JSON and hashes means that you can end up with :foo
and "foo" both in the same hash, which is bad. Use a mash so they are
always the same.

Closes facebook#115.

Signed-off-by: Phil Dibowitz <phil@ipom.com>
malmond77 pushed a commit that referenced this issue Jul 22, 2019
Moving data between JSON and hashes means that you can end up with :foo
and "foo" both in the same hash, which is bad. Use a mash so they are
always the same.

Closes #115.

Signed-off-by: Phil Dibowitz <phil@ipom.com>
@malmond77
Copy link
Contributor

malmond77 commented Aug 14, 2019

Now I re-read the code in #116 , I am perplexed. I am not confident that it actually does anything. outside of a minor formatting change, this is assigning state = Mash.new only to be usually overwritten with a hash from JSON.parse.

The mash structure is cool, but I am not sure it helps here. I think a simpler solution to this problem is to flatten the key .to_s in the write method. How does that sound?

@malmond77 malmond77 reopened this Aug 14, 2019
@jaymzh
Copy link
Collaborator Author

jaymzh commented Aug 14, 2019

no, you silly man. That's what mashes do... they treat string and symbol keys as the same thing. That's literally the point of them, so it always collapses them and we only ever write out strings, and always ever read in strings without constantly converting manually.

@jaymzh jaymzh closed this as completed Aug 15, 2019
@malmond77
Copy link
Contributor

I get that mashes handle keys differently. That's not the issue. The issue I'm raising is that simply assigning a new mash to state before trying to read a json file / falling back on an empty hash. The resulting data type of state is always a hash not a mash as you intended. Example:

$ irb
irb(main):001:0> require 'chef/mash'
=> true
irb(main):002:0> require 'json'
=> true
irb(main):003:0> m = Mash.new
=> {}
irb(main):004:0> m.class
=> Mash
irb(main):005:0> m = JSON.parse(File.read('/data/users/malmond/.taste-tester/prod/ref.json'))
=> {"port"=>5093, "ssl"=>true, "ssh"=>false, "logging"=>true, "bundle"=>"compatible", "ref"=>"bb50da1a52bd0e9e1ab1d03649ed27690445e092", "last_upload_time"=>"2019-08-15 10:24:41"}
irb(main):006:0> m.class
=> Hash
irb(main):007:0> m = {}
=> {}
irb(main):008:0> m.class
=> Hash

I don't think the code in #116 actually does anything. If anything, the solution here is even simpler:

     def write(key, val)
-      merge({ key => val })
+      merge({ key.to_s => val })
     end

This flattens all keys to strings, all the time.

@malmond77 malmond77 reopened this Aug 15, 2019
@jaymzh
Copy link
Collaborator Author

jaymzh commented Aug 15, 2019

Of course it's JSON... but it collapses all the keys. Every time. That's the point of mashes. What's written out is JSON, it's read into a mash, the keys merge properly. You CANNOT find a case with the current code where you'll get a :state and a "state" key, I'll bet money on it.

@jaymzh
Copy link
Collaborator Author

jaymzh commented Aug 15, 2019

irb(main):002:0> m = Mash.new
=> {}
irb(main):003:0> m[:stuff] = 'bar'
=> "bar"
irb(main):004:0> m.merge!(JSON.parse('{"stuff": "wonk"}'))
=> {"stuff"=>"wonk"}

This is the whole reason that Chef wrote Mash. Let Mash do that for you so you're not doing it yourself.

malmond77 added a commit to malmond77/taste-tester that referenced this issue Aug 19, 2019
This commit demonstrates a bug in state handling. Every modification involves a read, modify, write. This unit test mocks out just enough to show that a state file with ssl -> true is read correctly, changed, then produces an invalid json file with two entries in the hash with the same key. This is the key issue as reported in facebook#115.

```
[malmond@devvm1846.vll1 ~/local/taste-tester]
/opt/chefdk/embedded/bin/rspec .
F

Failures:

  1) TasteTester::State should serialize changes correctly
     Failure/Error: expect(@buffer.string).to eq('{"ssl":false}')

       expected: "{\"ssl\":false}"
            got: "{\"ssl\":true,\"ssl\":false}"

       (compared using ==)
     # ./spec/taste-tester_spec.rb:20:in `block (2 levels) in <top
     # (required)>'

Finished in 0.01544 seconds (files took 0.27334 seconds to load)
1 example, 1 failure

Failed examples:

rspec ./spec/taste-tester_spec.rb:6 # TasteTester::State should serialize changes correctly
```

Signed-off-by: Matthew Almond <malmond@fb.com>
malmond77 added a commit to malmond77/taste-tester that referenced this issue Aug 19, 2019
This is to fix the issue reported in facebook#115.

It is possible to modify `state.rb` to use Mash in a way that does merge keys properly. However this is more complex than neccessary. The existing code already uses `.to_s` on reads. Given a choice between the two approaches, I am chosing to keep the code simpler and remove the unneccessary import.

Signed-off-by: Matthew Almond <malmond@fb.com>
malmond77 added a commit to malmond77/taste-tester that referenced this issue Aug 19, 2019
This commit demonstrates a bug in state handling. Every modification involves a read, modify, write. This unit test mocks out just enough to show that a state file with ssl -> true is read correctly, changed, then produces an invalid json file with two entries in the hash with the same key. This is the key issue as reported in facebook#115.

```
[malmond@devvm1846.vll1 ~/local/taste-tester]
/opt/chefdk/embedded/bin/rspec .
F

Failures:

  1) TasteTester::State should serialize changes correctly
     Failure/Error: expect(@buffer.string).to eq('{"ssl":false}')

       expected: "{\"ssl\":false}"
            got: "{\"ssl\":true,\"ssl\":false}"

       (compared using ==)
     # ./spec/taste-tester_spec.rb:20:in `block (2 levels) in <top
     # (required)>'

Finished in 0.01544 seconds (files took 0.27334 seconds to load)
1 example, 1 failure

Failed examples:

rspec ./spec/taste-tester_spec.rb:6 # TasteTester::State should serialize changes correctly
```

Signed-off-by: Matthew Almond <malmond@fb.com>
malmond77 added a commit to malmond77/taste-tester that referenced this issue Aug 19, 2019
This is to fix the issue reported in facebook#115.

It is possible to modify `state.rb` to use Mash in a way that does merge keys properly. However this is more complex than neccessary. The existing code already uses `.to_s` on reads. Given a choice between the two approaches, I am chosing to keep the code simpler and remove the unneccessary import.

Signed-off-by: Matthew Almond <malmond@fb.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants