Don't persist unset fields on update #6199

Merged
merged 3 commits into from Dec 28, 2016

Projects

None yet

3 participants

@GawainLynch
Member
GawainLynch commented Dec 24, 2016 edited
  • Don't add a field to the persistence query if it is not an entity property

To reproduce the problem, the simplest ContentType:

koalas:
    name: Koalas
    singular_name: Koala
    fields:
        title:
            type: text
        slug:
            type: slug
            uses: title
        imagelist:
            type: imagelist

Test PHP code:

$repo = $app['storage']->getRepository('koalas');
$entity = $repo->getEntityBuilder()->getEntity();
$entity->set('title', 'Kenny Koala');
$entity->set('status', 'published');
$entity->set('slug', 'kenny-koala');
$repo->save($entity);

Result:

SQLSTATE[23000]: Integrity constraint violation: 1048 Column 'imagelist' cannot be null
@GawainLynch GawainLynch requested a review from rossriley Dec 24, 2016
@rossriley
Contributor

Do you not get the problem with a doesn't have a default value error.

Here's the create table I get from adding the above contenttype.

CREATE TABLE `bolt_koalas` (
  `id` int(11) NOT NULL AUTO_INCREMENT,
  `slug` varchar(128) COLLATE utf8_unicode_ci NOT NULL,
  `datecreated` datetime NOT NULL,
  `datechanged` datetime NOT NULL,
  `datepublish` datetime DEFAULT NULL,
  `datedepublish` datetime DEFAULT NULL,
  `username` varchar(32) COLLATE utf8_unicode_ci DEFAULT '',
  `ownerid` int(11) DEFAULT NULL,
  `status` varchar(32) COLLATE utf8_unicode_ci NOT NULL,
  `templatefields` longtext COLLATE utf8_unicode_ci NOT NULL COMMENT '(DC2Type:json_array)',
  `title` varchar(256) COLLATE utf8_unicode_ci DEFAULT '',
  `imagelist` longtext COLLATE utf8_unicode_ci NOT NULL COMMENT '(DC2Type:json_array)',
  PRIMARY KEY (`id`),
  KEY `IDX_AEF722B989D9B62` (`slug`),
  KEY `IDX_AEF722BAFBA6FD8` (`datecreated`),
  KEY `IDX_AEF722BBE74E59A` (`datechanged`),
  KEY `IDX_AEF722BA5131421` (`datepublish`),
  KEY `IDX_AEF722BB7805520` (`datedepublish`),
  KEY `IDX_AEF722B7B00651C` (`status`)
) ENGINE=InnoDB DEFAULT CHARSET=utf8 COLLATE=utf8_unicode_ci;

What are you seeing?

@GawainLynch
Member

This:

CREATE TABLE `bolt_koalas` (
  `id` int(11) NOT NULL AUTO_INCREMENT,
  `slug` varchar(128) COLLATE utf8_unicode_ci NOT NULL,
  `datecreated` datetime NOT NULL,
  `datechanged` datetime NOT NULL,
  `datepublish` datetime DEFAULT NULL,
  `datedepublish` datetime DEFAULT NULL,
  `username` varchar(32) COLLATE utf8_unicode_ci DEFAULT '',
  `ownerid` int(11) DEFAULT NULL,
  `status` varchar(32) COLLATE utf8_unicode_ci NOT NULL,
  `templatefields` longtext COLLATE utf8_unicode_ci NOT NULL COMMENT '(DC2Type:json_array)',
  `title` varchar(256) COLLATE utf8_unicode_ci DEFAULT '',
  `imagelist` longtext COLLATE utf8_unicode_ci NOT NULL COMMENT '(DC2Type:json_array)',
  PRIMARY KEY (`id`),
  KEY `IDX_AEF722B989D9B62` (`slug`),
  KEY `IDX_AEF722BAFBA6FD8` (`datecreated`),
  KEY `IDX_AEF722BBE74E59A` (`datechanged`),
  KEY `IDX_AEF722BA5131421` (`datepublish`),
  KEY `IDX_AEF722BB7805520` (`datedepublish`),
  KEY `IDX_AEF722B7B00651C` (`status`)
) ENGINE=InnoDB AUTO_INCREMENT=20 DEFAULT CHARSET=utf8 COLLATE=utf8_unicode_ci;

So me confused.

@rossriley
Contributor

Yes, so looks like we have identical table structure, I'm just intrigued how your code saves without an error then since the generated query is:

INSERT INTO bolt_koalas (slug, datecreated, datechanged, datepublish, datedepublish, ownerid, status, title) VALUES(?, ?, ?, ?, ?, ?, ?, ?)' with params ["kenny-koala", "2016-12-24 11:46:21", "2016-12-24 11:46:21", null, null, null, "published", "Kenny Koala"]:

Table structure should block on both templatefields and imagelist which according to the structure can't be null.

@GawainLynch
Member
GawainLynch commented Dec 24, 2016 edited

When I get to $result = $querySet->execute();, the QueryBuilder in $querySet casts to the following string:

INSERT INTO bolt_koalas (slug, datecreated, datechanged, datepublish, datedepublish, ownerid, status, title) VALUES(:slug, :datecreated, :datechanged, :datepublish, :datedepublish, :ownerid, :status, :title)

Looking at the QueryBuilder object, the $sqlParts['set'] array doesn't include the image field, neither does $sqlParts['set']

So maybe the usual MySQL version !==?

@rossriley
Contributor
rossriley commented Dec 24, 2016 edited

what's your output from SELECT @@sql_mode;

@GawainLynch
Member
+--------------------------------------------+
| @@sql_mode                                 |
+--------------------------------------------+
| NO_AUTO_CREATE_USER,NO_ENGINE_SUBSTITUTION |
+--------------------------------------------+
@rossriley
Contributor

Mine was STRICT_TRANS_TABLES,NO_ENGINE_SUBSTITUTION and by process of elimination it seems that having STRICT_TRANS_TABLES on leads to the null value error.

Info here: https://dev.mysql.com/doc/refman/5.6/en/sql-mode.html#sql-mode-strict

I'm not sure what the default setting is, mine is homebrew installed on OSX and a quick check through some other servers which are default distro package installed seems that an empty sql mode is more common, so it could just be that strict mode is an unusual setting to be on.

@GawainLynch
Member

Yeah STRICT_TRANS_TABLES,NO_AUTO_CREATE_USER,NO_ENGINE_SUBSTITUTION throws an exception here too.

@GawainLynch
Member

So what about:

if (!isset($entity->$fieldName) && $entity->getId() !== null) {

If the ID exists it is an update, and won't have the same issue, as we're just being specific about what columns in the row to update.

That is actually the use-case that was biting me in the first place.

@rossriley
Contributor

OK, yes, that's simpler than I was thinking, so yes probably best in a bugfix branch.

We can perhaps address partial saves for creates too in the future since that is something that will also probably come up at some point.

@GawainLynch
Member

Updated

@bobdenotter
Member

Works like a charm! 👍

@bobdenotter bobdenotter merged commit f4ca640 into release/3.2 Dec 28, 2016

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@bobdenotter bobdenotter deleted the hotfix/entity-persist branch Dec 28, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment