? Success
Referenced as Related to: # 3087

User tests: Successful: Unsuccessful:

avatar gunjanpatel
gunjanpatel
15 Oct 2014
  • Allow null value for, #__content table images, urls, attribs and metadata columns.
  • Please follow testing guidelines from here #3087

Votes

# of Users Experiencing Issue
1/1
Average Importance Score
3.00

avatar gunjanpatel gunjanpatel - open - 15 Oct 2014
avatar jissues-bot jissues-bot - change - 15 Oct 2014
Labels Added: ?
avatar Bakual
Bakual - comment - 15 Oct 2014

Copying issue text:

Joomla Version: 3.2.2
PostgreSQL version: 9.1+
OS: Ubuntu 12.04 and 13.10
PHP version: 5.3.10 (ubuntu 12.04) and 5.5.3 (Ubuntu 13.10)

Error
Save failed with the following error: Database query failed (error # %s): %s SQL=INSERT INTO "rvyni_content" ("id","title","alias","introtext","fulltext","state","catid","created","created_by","created_by_alias","modified","publish_up","publish_down","images","urls","attribs","version","metakey","metadesc","access","metadata","featured","language","xreference") VALUES (0,'Test Title','test-title','

Test Description

','',1,2,'2014-02-11 14:57:35',509,'','1970-01-01 00:00:00','2014-02-11 14:57:35','1970-01-01 00:00:00','{"image_intro":"","float_intro":"","image_intro_alt":"","image_intro_caption":"","image_fulltext":"","float_fulltext":"","image_fulltext_alt":"","image_fulltext_caption":""}','{"urla":false,"urlatext":"","targeta":"","urlb":false,"urlbtext":"","targetb":"","urlc":false,"urlctext":"","targetc":""}','{"show_title":"","link_titles":"","show_tags":"","show_intro":"","info_block_position":"","show_category":"","link_category":"","show_parent_category":"","link_parent_category":"","show_author":"","link_author":"","show_create_date":"","show_modify_date":"","show_publish_date":"","show_item_navigation":"","show_icons":"","show_print_icon":"","show_email_icon":"","show_vote":"","show_hits":"","show_noauth":"","urls_position":"","alternative_readmore":"","article_layout":"","show_publishing_options":"","show_article_options":"","show_urls_images_backend":"","show_urls_images_frontend":""}',1,'','',1,'{"robots":"","author":"","rights":"","xreference":""}',0,'*','') RETURNING id

avatar brianteeman brianteeman - change - 15 Oct 2014
Category MS SQL Postgresql SQL
avatar wilsonge
wilsonge - comment - 16 Oct 2014

Is there a reason why this can be null? As I recall all these columns are json encoded which means that on save at minimum they should have a {} tag in them to reflect they are empty!

avatar nicksavov nicksavov - change - 16 Oct 2014
Labels Added: ?
avatar luredweb
luredweb - comment - 17 Oct 2014

@test - It's working :)

This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/4687.

avatar luredweb luredweb - test_item - 17 Oct 2014 - Tested successfully
avatar alikon alikon - test_item - 17 Oct 2014 - Tested successfully
avatar alikon
alikon - comment - 17 Oct 2014

@test success
@wilsonge from data design POV you should ask :'every article item have an image?' I think the answer is no

This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/4687.

avatar brianteeman
brianteeman - comment - 17 Oct 2014

Multiple good tests thanks - setting to RTC

This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/4687.

avatar brianteeman brianteeman - change - 17 Oct 2014
Status Pending Ready to Commit
avatar wilsonge
wilsonge - comment - 17 Oct 2014

Of course every item won't. But because it's json_encoded - even a null value means that the actual value stored in the database will NOT be null. See example 2 on the PHP documentation with an empty array http://php.net/manual/en/function.json-encode.php

$b = array();

echo "Empty array output as array: ", json_encode($b), "\n";

giving the output

Empty array output as array: []
avatar wilsonge wilsonge - test_item - 17 Oct 2014 - Tested unsuccessfully
avatar wilsonge
wilsonge - comment - 17 Oct 2014

I'm unsetting this from RTC and setting to Needs Review for a bit because I want someone else to look over this. Something is very wrong here for the reasons I described - and I don't want us to find in 6 months time this masked some horrendous bigger problem

avatar wilsonge wilsonge - change - 17 Oct 2014
Status Ready to Commit Needs Review
avatar wilsonge wilsonge - test_item - 17 Oct 2014 - Not tested
avatar gunjanpatel
gunjanpatel - comment - 17 Oct 2014

Thanks @wilsonge Valid point. Needs review. Thanks.

avatar phproberto
phproberto - comment - 17 Oct 2014

I agree with @wilsonge

This is not the right fix.

avatar alikon
alikon - comment - 17 Oct 2014

Just not consider only the code perspective,but think also in term of SQL constraints, surely there is a bug on the code side for having a null value on these fields, but the nature of that kind of fields I think is nullable. I agree too more reviews needed

avatar Bakual
Bakual - comment - 17 Oct 2014

Just not consider only the code perspective,but think also in term of SQL constraints, surely there is a bug on the code side for having a null value on these fields, but the nature of that kind of fields I think is nullable. I agree too more reviews needed

According to Georges argument, the field never should have a null value and thus indeed shouldn't be nullable. Instead it should be an empty JSON string {} and this also should then actually be the default value.

avatar wilsonge
wilsonge - comment - 17 Oct 2014

Exactly! The fact we have more than just an empty string in the original issue means that something is significantly more wrong.

avatar gunjanpatel
gunjanpatel - comment - 17 Oct 2014

Actually, we will have to change type from TEXT to VARCHAR.
For example: ALTER TABLEglt5s_contentCHANGEimagesimagesVARCHAR( 5120 ) NOT NULL DEFAULT '{}'

This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/4687.

avatar wilsonge
wilsonge - comment - 17 Oct 2014

Presumably as people use this on Mysql without any issues day in day out that would be for postgres only tho?

avatar gunjanpatel
gunjanpatel - comment - 17 Oct 2014

Yup, this is for example. I will remove alter queries from mysql file, actually whole mysql file and will change postgres alter query with help of @alikon :)

avatar gunjanpatel
gunjanpatel - comment - 17 Oct 2014

I just revert MySQL changes. :)

avatar LLNet LLNet - test_item - 17 Oct 2014 - Tested successfully
avatar wilsonge
wilsonge - comment - 17 Oct 2014

Guys even so the bigger question is that in the bug report we can see there is data that should be being written to those columns (I highlighted it in my big explanation post earlier) so why does postgres seem to think that big json expression is equal to null?

avatar alikon
alikon - comment - 17 Oct 2014

this issue arise only on frontend content submission on backend it works as for #3087
so i think is dued by some malformed json expression triggered from "frontend code"
postgresql works well with json as i know, mysql is a lot more permissive than postgresql on SQL standards
and this is a clear example
postgesql can helps us to trap this kind of bugs
;)



This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/4687.

avatar wilsonge
wilsonge - comment - 17 Oct 2014

So in that case can we close this PR all together and fix the malformed json?

avatar brianteeman brianteeman - change - 18 Oct 2014
Title
[fix #3087] Allow NULL for #__content columns
Allow NULL for #__content columns
avatar wilsonge
wilsonge - comment - 18 Oct 2014
"q8dzo_content" ("title","alias","introtext","fulltext","state","catid","created","created_by","created_by_alias","publish_up","publish_down","images","urls","version","metakey","metadesc","access","featured","language") VALUES ('Foobar','balh','

asdfadsfasdf

','',1,14,'2014-10-19 16:09:48',371,'','2014-10-19 16:09:48','1970-01-01 00:00:00','{"image_intro":"","image_intro_alt":"","image_intro_caption":"","float_intro":"","image_fulltext":"","image_fulltext_alt":"","image_fulltext_caption":"","float_fulltext":""}','{"urla":false,"urlatext":"","targeta":"","urlb":false,"urlbtext":"","targetb":"","urlc":false,"urlctext":"","targetc":""}',1,'','',1,0,'*') RETURNING id

OK So first bug is that when images and urls are turned off we have no images or url fields in the frontend hence why urls and images will throw an error. I guess we need to form a empty blank json variable in that scenario (imo we should do that in the table class tho - as despite it works on mysql it doesn't make it 'right').

With that parameter turned on to show the url and images fields I get the query error above. Note that urls and images appear to have valid json. So I think we can rule out those from our issue list. So now working on spot the syntax error :P

avatar Bakual
Bakual - comment - 18 Oct 2014

We can actually do both ways. If you set the correct default value ({}) in the database like proposed in this PR, you can save a few lines in the code. However it still could make sense to also make sure the code always sets the value.

avatar wilsonge
wilsonge - comment - 18 Oct 2014

Makes sense. I think mysql must treat an empty string as NOT NULL otherwise the tags in #4815 wouldn't have ever saved properly since 3.2.

So looking at the above query even when we show urls and images we still don't have the metadata and attribs fields. So I guess that is the issue - postgres is just much more strict with it's definition of null than mysql. So do we have these fields as hidden, do we set them in the model? Or what? Because even though this works in mysql i guess having an empty field isn't optimal there either?

avatar Bakual
Bakual - comment - 19 Oct 2014

@gunjanpatel Can you update the PR so it contains the proper default values ('{}') instead of NULL?
Imho it could actually be changed for the other databases as well, becaus it would be more accurate.

I think mysql must treat an empty string as NOT NULL otherwise the tags in #4815 wouldn't have ever saved properly since 3.2.

Yep, that's exactly the issue. I think you can set MySQL to a strict mode as well and it will refuse to store. I had that happen when I set up MariaDB (basically same as MySQL) once which by default seems to run in a strict mode.

avatar gunjanpatel
gunjanpatel - comment - 20 Oct 2014

@Bakual I have updated code and set {} as default for postgres.
Thanks to @alikon and @wilsonge

avatar Bakual
Bakual - comment - 24 Oct 2014

After reading a bit about this stuff, I came to the conclusion that it's better to allow NULL values for those fields.
The reason is that MySQL doesn't allow to set a default value for TEXT fields. While it would work for PostgreSQL and MS SQL to set a default value of {}, it would fail for MySQL.

I'd rather have it consistent across the databases and thus allow NULL in all three supported databases. For our code it shouldn't matter anyway.

What do you think?

avatar gunjanpatel
gunjanpatel - comment - 28 Oct 2014

I think it's a good idea. :+1: If we agree on it then I will roll back gunjanpatel@170274b changes.

avatar Bakual
Bakual - comment - 28 Oct 2014

I would prefer it being nullable.

I was tinkering a bit yesterday with that: https://github.com/Bakual/joomla-cms/compare/SqlTextFieldsNullable which would make all text fields nullable in MySQL and PostgreSQL.
I haven't had time yet to test it so didn't create a PR yet.

avatar wilsonge
wilsonge - comment - 28 Oct 2014

I really still feel like in the majority of cases it is just poor php coding by us rather than cases where these fields should be nullable.

avatar Bakual
Bakual - comment - 28 Oct 2014

I really still feel like in the majority of cases it is just poor php coding by us rather than cases where these fields should be nullable.

It's both. Poor database design and poor coding :smile:
From a database design perspective, text fields are either null or they have to contain a value. Imho there is no point in storing an empty string. It only consumes memory without adding value.

avatar wilsonge
wilsonge - comment - 28 Oct 2014

Well theoretically it should be more than just a empty json string. It should contain all the default values of the parameters we are not showing!

avatar sovainfo
sovainfo - comment - 28 Oct 2014

It is not considered bad database design! In general it is not adviced to allow null for any column. At the minimum avoid null for PK, FK, numeric columns and colums involved in query selection criterea.
When allowed make sure your code deals with null values as expected. A column with a domain of null, a, b, c; a query with condition 'column not equal b' doesn't return null rows. With a domain of '', a, b, c; rows with '' are included in the result as expected by many. Ofcourse 'column is null or column not equal b' would be the correct condition.

Don't know enough about JSON, but don't like storing empty values there either. There is probably a good reason to waste so much space!

avatar Bakual
Bakual - comment - 28 Oct 2014

It is not considered bad database design! In general it is not adviced to allow null for any column. At the minimum avoid null for PK, FK, numeric columns and colums involved in query selection criterea

I agree with that. If possible don't allow null and provide a default value.
Here we're only talking about text (and mediumtext) fields. They have the issue that in MySQL, you can't define a default value for text fields and it uses an empty string instead of null if null isn't allowed. In PostgreSQL it doesn't do that and you'd have to define a default value or make sure the code always passes a value (which it currently doesn't).
So we either can define a default value in PostgreSQL which would make the schemas different from MySQL, or we can allow null which would work for all databases.

The typical fields it possibly affects are:

  • description
  • params
  • metakey
  • metadata
  • metadesc
  • urls
  • images

I don't think those are used anywhere as keys or selection criterias.

In com_content there are two additional text fields introtext and fulltext. One could argue what to do with those. Imho we could leave them as I'm quite convident the code makes sure those are set always and it actually would make sense that the query fails if those are not set.

avatar sovainfo
sovainfo - comment - 28 Oct 2014

No argument there. Would like Joomla to use the at minimum rule for the database. Let the model decide to respect null values or not.

avatar brianteeman
brianteeman - comment - 27 Nov 2014

Where are we with this one? @bakual @sovainfo @wilsonge

This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/4687.

avatar sovainfo
sovainfo - comment - 27 Nov 2014

Looks like it is @Bakual @sovainfo against @wilsonge @phproberto as in dropping not null constraint vs keeping it.
No solution has been provided for keeping it and dropping it is rejected. Doesn't that normally mean it is closed in 4 weeks time if no solutions is provided?

avatar Bakual
Bakual - comment - 27 Nov 2014

I've pinged PLT to have a look.
Basically there are three ways to solve it:

  1. Define a default value -> isn't possible in MySQL since it doesn't allow to set a default value for text fields. But would work for the other DBs. It would however make the schemas different.
  2. Allow NULL to be stored. Should work in all DBs. However I don't know if there was a reason to not allow NULL for those fields.
  3. Make sure in the code that there is always a value stored. This would work for all DBs obviously but is likely to break sooner or later again and is a lot of work to find all instances.

This PR uses approach 1.
I did a branch to see how it could be done for approach 2 (Bakual@joomla:staging...Bakual:SqlTextFieldsNullable) and George did some PRs with approach 3.

avatar wilsonge
wilsonge - comment - 27 Nov 2014

Having had some time to think about it my argument is that actually to stop null columns being null etc. is exactly the point of the JTable::check() method. This is probably a symptom that we need to add in much more rigorous checks there for the data we are storing into the database.

avatar wilsonge
wilsonge - comment - 28 Nov 2014

Whipped this up before I go to bed #5230 (note haven't tested it but will do tomorrow) but gives you an idea of how I see we should go

avatar alikon
alikon - comment - 28 Nov 2014

the basic check #5230 does his job well, is simple, only in a logical place adn fix the #3087 issue
and could be enough for let 3.4.0 run better on postgresql
so, is a temporary fix we always need to clearly define what exctaly is the nature of these kind of fields, and how to manage, but i think this could take more time...

avatar Bakual
Bakual - comment - 28 Nov 2014

I still wonder why those text fields are defined as NOT NULL to begin with. Nobody seems to know...

avatar wilsonge
wilsonge - comment - 29 Nov 2014

Well because they shouldn't ever be null. They should have a full json string. Even my thing is a bit of a hacky way around it - really we should pick out all the defaults and store them

avatar Bakual
Bakual - comment - 29 Nov 2014

NULL basically means undefined, which would be exactly the case if the form doesn't define it.
Our code would threat it the same way as if it is empty.
I have a feeling that we make our life harder than needed by not allowing NULL for those fields.

we should pick out all the defaults and store them

We don't have any default values there. It's all empty by default :wink:

avatar sovainfo
sovainfo - comment - 29 Nov 2014

@wilsonge What makes you say they should never be null. What part of the application requires them to have values. Only when the application requires it the NOT NULL is justified. When implementing the minimum rule as mentioned above these would not be justified. So far all applications can cope with these fields having no value entered yet. That makes the NOT NULL unjustified.

avatar wilsonge
wilsonge - comment - 29 Nov 2014

No I disagree. These are the parameters for the articles. They should always have values. We then have emergency fall backs in the article when we add default values when we retrieve the parameter values (stored in the attribs) doing stuff like $param->get('foobar', 'default');. If we could (and in an ideal world we should) expect the value to always be defined then we wouldn't need this. The reason it's not always set (of course this is slightly biased because sometimes there are new params etc.) is because when the database has null values for the fields because we're not storing anything there.

Ditto for images. If we aren't showing images then we should have this stored in the database not just guessed at when the entire images column is empty

We should only need to define the fallbacks once. And that place is in the form.xml - hence why I stated above my thing is a hack because it doesn't actually solve that problem - although it solves the immediate postgres issue.

avatar Bakual
Bakual - comment - 30 Nov 2014

We don't have any default values for images, urls, metadata and attribs. All parameters are saved with an empty value, if anything is stored at all.
The default values are not set in the form.xml (and should not set there!). They are set in the component options or there are (rightfully) no default values at all.

It absolutely makes no difference if the value in the database is NULL, an empty string or a JSON string with parameters defined with empty values.

avatar sovainfo
sovainfo - comment - 30 Nov 2014

First of all, I object to these attributes of an article being called parameters.
Second, the question was what makes you say they should have values! Just repeating the statement doesn't explain why you say it and doesn't make your statement true!

For example: images: The business logic of the application doesn't require an article to have images. When no images are provided the application functions just as well. Compare this to the username and email of the user. The business logic within Joomla requires these to be present. A user is considered invalid without them. Those attributes should have NOT NULL. Which makes required to be entered upon creation of a user. It is not allowed to provide them later. Compare that to images: they can be specified at any time, added or removed. An article is considered valid with or without them. That makes them OPTIONAL, the attribute should allow NULL, respecting the minimum rule!

avatar Bakual Bakual - close - 2 Dec 2014
avatar Bakual Bakual - change - 2 Dec 2014
Status Needs Review Closed
Closed_Date 0000-00-00 00:00:00 2014-12-02 20:26:17
avatar Bakual
Bakual - comment - 2 Dec 2014

Merged this PR as is into staging. It will fix the issue at hand even if it's kind of a workaround :wink:
Thanks for all the work, discussing and testing!

Add a Comment

Login with GitHub to post a comment