| 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! | 08:06 |
| cjwatson | guruprasad: Sure | 09:27 |
| 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:31 |
| 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:32 |
| 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:33 |
| 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:34 |
| cjwatson | guruprasad: It's not just that you need to use storm.databases.postgres.JSONVariable instead (assuming this is a jsonb column)? | 09:35 |
| cjwatson | (or .JSON) | 09:36 |
| guruprasad | Ah! | 09:37 |
| 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:39 |
| cjwatson | Might be worth a docstring patch to storm though | 09:40 |
| guruprasad | I can make that change and submit it for review - what do you think the docstring should say and which one? | 09:41 |
| cjwatson | probably all the involved classes should have docstringa that say what kind of columns they're meant to be used with | 09:42 |
| guruprasad | Sure. I will do that and propose the changes soon. | 09:44 |
| guruprasad | Thanks for the help! | 09:44 |
| 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 :) | 09:52 |
Generated by irclog2html.py 2.7 by Marius Gedminas - find it at mg.pov.lt!