[08:06] <guruprasad> cjwatson, I have run into what looks like a Storm bug/issue and want to talk to you about it before attempting to submit a fix. Do you have a few minutes to talk about it? Also, is there a better place for this discussion - I checked #storm but it is not present on irc.libera.chat.
[08:06] <guruprasad> Thanks in advance for your time!
[09:27] <cjwatson> guruprasad: Sure
[09:31] <guruprasad> We have run into what looks like a bug in the JSONVariable class in Storm. In error scenario, the exception in https://bazaar.launchpad.net/~launchpad-committers/storm/lp/view/head:/storm/variables.py#L643 is hit because the value is already a dict instead of stringified JSON.
[09:32] <guruprasad> I happened to find https://git.launchpad.net/launchpad/tree/lib/lp/services/database/stormexpr.py#n307, the `_loads` method in particular, where you have mentioned that psycopg >= 2.5 already handles the conversion to JSON.
[09:33] <guruprasad> In case of Launchpad, this happens when trying to set a JSON attribute (metadata_overrides) on IArchive via the webservice API and then using `archive.metadata_overrides` where `archive` is the model instance.
[09:34] <guruprasad> Strangely, this happens only in the case of 'do a web service call to set it and then check the model instance' and not the 'set it on the model instance and check it after that' operation
[09:35] <cjwatson> guruprasad: It's not just that you need to use storm.databases.postgres.JSONVariable instead (assuming this is a jsonb column)?
[09:36] <cjwatson> (or .JSON)
[09:37] <guruprasad> Ah!
[09:39] <cjwatson> storm.variables.JSONVariable / storm.properties.JSON is only for text columns that happen to contain JSON, not for the native pg types
[09:39] <guruprasad> This is a jsonb column in postgres and the model uses storm.locals.JSON
[09:39] <cjwatson> Yeah, use the one in s.d.p instead and you should be fine
[09:40] <cjwatson> Might be worth a docstring patch to storm though
[09:41] <guruprasad> I can make that change and submit it for review - what do you think the docstring should say and which one?
[09:42] <cjwatson> probably all the involved classes should have docstringa that say what kind of columns they're meant to be used with
[09:44] <guruprasad> Sure. I will do that and propose the changes soon.
[09:44] <guruprasad> Thanks for the help!
[09:52] <cjwatson> I was 90% sure I knew what the problem was the moment I read "JSON", so that's a good sign it needs better docs :)