Skip to content

Conversation

@tdealtry
Copy link
Contributor

Also a prettyication change of

{"$0":"./main", "$1":"WCSim_NHitsOnly_FlatOut_1ThreadPerTool"

instead of

{"$0":"./main" ,"$1":"WCSim_NHitsOnly_FlatOut_1ThreadPerTool"

I'm sorry, but the comma was definitely in the wrong place!!

Closes #19

@tdealtry
Copy link
Contributor Author

tdealtry commented Oct 9, 2024

Just to follow up. I tried the current main branch & am still seeing the issue. e.g. reduced example output

import json
import tempfile

config_fixed = '{"$0":"./main", "in_wcsim":{"first_event":"0" } }'
jconfig = json.loads(config_fixed)
print(jconfig)
config_wrong = '{"$0":"./main" ,"in_wcsim":"{"first_event":"0" }" }'
jconfig = json.loads(config_wrong)
print(jconfig)

Python throws this on json.loads(config_wrong)

json.decoder.JSONDecodeError: Expecting ',' delimiter: line 1 column 31 (char 30)

The issue is that the nested JSON block (i.e. the value of in_wcsim in this example) is quoted

@tdealtry
Copy link
Contributor Author

tdealtry commented Oct 16, 2024

In order to make the review of this easier, here's a little script

#include "Store.h"

#include <iostream>
#include <string>

int main() {
  //Setup a store & fill it
  ToolFramework::Store  s1;
  s1.Set("int", 1);
  s1.Set("float", 134.1);

  //Nest s1 inside s2
  ToolFramework::Store s2;
  std::string s1_str = "";
  s1 >> s1_str;
  s2.Set("store", s1_str);
  s2.Print();

  //Save s2 into a string
  std::string s2_str = "";
  s2 >> s2_str;
  std::cout << s2_str << std::endl;

  //Use this s2 string to create s3
  ToolFramework::Store s3;
  s3.JsonParser(s2_str);
  s3.Print();

  //save s3 into a string
  std::string s3_str = "";
  s3 >> s3_str;
  std::cout << s3_str << std::endl;
}

If you save it as test_store.cxx, then do this

g++ -std=c++14 test_store.cxx -o test_store -I include/ -L lib/ -lStore && ./test_store

Then, with this PR as of submission day, we have

store => "{"float":134.1, "int":1}"
{"store":{"float":134.1, "int":1}}
store => {"float":134.1, "int":1}
{"store":{"float":134.1, "int":1}}
  • the output of s2 & s3 to string is consistent most importantly, valid JSON
  • the output of s2 & s3 .Print() is inconsistent (hints at another bug)

With this PR + a merge of main today

store => "{"float":134.1, "int":1}"
{"store":{"float":134.1, "int":1}}
store => {"float":134.1, "int":1}
{"store":{"float":134.1, "int":1}}
  • Same as original PR

With the main branch as of September 18th (6f76253)

store => "{"float":134.1 ,"int":1 }"
{"store":"{"float":134.1 ,"int":1 }" }
int => 1
store => "{"
{"int":1 ,"store":"{" }
  • a mess

With the main branch as of today (e1a1352)

store => "{"float":134.1 ,"int":1 }"
{"store":"{"float":134.1 ,"int":1 }" }
store => {"float":134.1 ,"int":1 }
{"store":{"float":134.1 ,"int":1 } }

Better than before, but the only way to produce a valid JSON string is by exporting the Store to a JSON string, then parsing it with a different Store...

TLDR; there is still a bug in Store::operator>>() this PR fixes it.
(There's also a still a bug in Store::Print() this doesn't attempt to fix it)

@tdealtry
Copy link
Contributor Author

Just to flag that this is still an issue. I've just tried with current main & this PR merged with main

Current main

store => "{"float":134.1 ,"int":1 }"
{"store":"{"float":134.1 ,"int":1 }" }
store => {"float":134.1 ,"int":1 }
{"store":{"float":134.1 ,"int":1 } }

The issue is on line 2 - the nested store is wrapped in extra quotes (i.e. "{"float":134.1 ,"int":1 }" should be {"float":134.1 ,"int":1 }.
The only way to produce a valid JSON string when using nested Store's is by exporting the Store to a JSON string, then parsing it with a different Store...

This PR

store => "{"float":134.1, "int":1}"
{"store":{"float":134.1, "int":1}}
store => {"float":134.1, "int":1}
{"store":{"float":134.1, "int":1}}

Produces valid JSON always when using JSONParser()
(The result of Print() is inconsistent, as in current main. That can be fixed in another PR, if it matters, and when the "correct" style for Print() is decided)

Is there anything else I can do to show you that this is a real bug & the fix works? This bug is complicating TriggerApp work on HK, so we are eager for a fix

@brichards64
Copy link
Contributor

fixed in another PR with a different method

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 this pull request may close these issues.

Invalid JSON

2 participants