Default value of JSONEncodedDict columns should be { } instead of "{ }"

Bug #1270146 reported by Max Lobur
6
This bug affects 1 person
Affects Status Importance Assigned to Milestone
Ironic
Fix Released
Low
Max Lobur

Bug Description

There are a bunch of places where we putting "{ }" as default to the JSONEncodedDict column:
https://github.com/openstack/ironic/blob/master/ironic/db/sqlalchemy/api.py#L473
https://github.com/openstack/ironic/blob/master/ironic/db/sqlalchemy/api.py#L413
https://github.com/openstack/ironic/blob/master/ironic/db/sqlalchemy/api.py#L297
https://github.com/openstack/ironic/blob/master/ironic/db/sqlalchemy/api.py#L295
https://github.com/openstack/ironic/blob/master/ironic/db/sqlalchemy/api.py#L293

This leads to writing the db value as ' "{ }" ' (https://github.com/openstack/ironic/blob/master/ironic/db/sqlalchemy/models.py#L59)
When the object is deserialized from the DB the property becomes string instead of dict, which may cause unexpected errors (due to interfaces mismatch). Currently we
don't have any problems with this because the UOM tries to deserialize the dict value again (https://github.com/openstack/ironic/blob/master/ironic/objects/utils.py#L68-L69)
But this looks like a workaround so it's better to fix the original problem

This is how it looks like in DB (Note that non-empy extra serialized as expected {} whereas empty extra is "{}") http://paste.openstack.org/show/62103/

I propose the following solution:
1. Remove all the places from DB API where JSONEncodedDict default is defined (all mentioned above)
2. Change JSONEncodedDict implementation to

    def process_bind_param(self, value, dialect):
        if value is None:
            value = dict()
        value = json.dumps(value)
        return value

Max Lobur (max-lobur)
summary: - Default value of JSONEncodedDict colums should be { } instead of "{ }"
+ Default value of JSONEncodedDict columns should be { } instead of "{ }"
Revision history for this message
OpenStack Infra (hudson-openstack) wrote : Fix proposed to ironic (master)

Fix proposed to branch: master
Review: https://review.openstack.org/68413

Changed in ironic:
status: Triaged → In Progress
Max Lobur (max-lobur)
Changed in ironic:
importance: Medium → Low
description: updated
Revision history for this message
OpenStack Infra (hudson-openstack) wrote : Fix merged to ironic (master)

Reviewed: https://review.openstack.org/68413
Committed: https://git.openstack.org/cgit/openstack/ironic/commit/?id=48269aac77efce9f05770c55986462c5c941eece
Submitter: Jenkins
Branch: master

commit 48269aac77efce9f05770c55986462c5c941eece
Author: Max Lobur <email address hidden>
Date: Wed Jan 22 08:53:26 2014 -0500

    Add JSONEncodedType with enforced type checking

    Add abstract base class for JSONEncoded properties which: 1. Does type
    checking to make sure we're serializing what we really expect
    2. Ensures that default value of the field matches default value of
    that type, so we have consistent interface. This patch creates two child
    classes JSONEncodedDict and JSONEncodedList and updates SA models
    correspondingly.

    Change-Id: I528ac64e998e9a874feb16709d7617feb4b09c8a
    Closes-Bug: #1270146

Changed in ironic:
status: In Progress → Fix Committed
Thierry Carrez (ttx)
Changed in ironic:
milestone: none → icehouse-3
status: Fix Committed → Fix Released
Thierry Carrez (ttx)
Changed in ironic:
milestone: icehouse-3 → 2014.1
To post a comment you must log in.
This report contains Public information  
Everyone can see this information.

Other bug subscribers

Remote bug watches

Bug watches keep track of this bug in other bug trackers.