Split binary to booleans #203

danopz · 2017-07-10T22:29:58Z

Since no one wrote on #191 I tried myself to map the status to boolean types. I already changed the code to use the new boolean fields instead of the status.
I don't really know what to do with StatusFlag::typeToStatus - if we should completely remove this one and somehow fix the code belongs to this, or move this to later and destruct this status in ItemMapper (see added comment). This would also change methods signatures for many methods.
I didn't find a way how we could add a data migraion from status to the new boolean fields, I just found asome docs in OC but it looks like in NC we don't have this feature - so how should we migrate?

I'm submitting this for the questions and also for a first review.

Todo

Drop unit tests ,write integration tests
Migration/Repair script
Completely remove status or map on some code parts?
Fix the integer/boolean stuff in the queries

sonologic · 2017-07-11T11:30:56Z

I was already working on it (assigned it to myself), got about as far as you did :) if you want to work on it further, i can look at some of the other bugs (if Bernhard hasn't fixed them all before that :). Bernhard pointed me to the docs for migration, its 'repair-steps' in info.cml iirc. At work now, don't have this info at hand.

…

On 11 July 2017 00:29:58 CEST, Daniel Opitz ***@***.***> wrote: Since no one wrote on #191 I tried myself to map the status to boolean types. I already changed the code to use the new boolean fields instead of the status. I don't really know what to do with StatusFlag::typeToStatus - if we should completely remove this one and somehow fix the code belongs to this, or move this to later and destruct this status in ItemMapper (see added comment). This would also change methods signatures for many methods. I didn't find a way how we could add a data migraion from status to the new boolean fields, I just found asome [docs in OC](https://doc.owncloud.org/server/10.0/developer_manual/app/schema.html) but it looks like in [NC we don't have this feature](https://docs.nextcloud.com/server/12.0/developer_manual/app/schema.html) - so how should we migrate? I'm submitting this for the questions and also for a first review. Todo --- - [ ] Fix tests - [ ] Migration script - [ ] Completely remove status or map on some code parts? You can view, comment on, or merge this pull request online at: #203 -- Commit Summary -- * replaced old status with 2 flags for unread and starred -- File Changes -- M lib/Db/FeedMapper.php (12) M lib/Db/Item.php (53) M lib/Db/ItemMapper.php (49) M lib/Db/Mysql/ItemMapper.php (21) M lib/Fetcher/FeedFetcher.php (2) M lib/Service/ItemService.php (9) -- Patch Links -- https://github.com/nextcloud/news/pull/203.patch https://github.com/nextcloud/news/pull/203.diff -- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: #203

-- Sent from my Android device with K-9 Mail. Please excuse my brevity.

sonologic · 2017-07-11T17:04:06Z

Looks like we basically did the same things, except I used true/false in sql where you used 1/0. I tend to prefer true/false.

Anyway, if you want to continue to work on it that would be very welcome!

I started to investigate 'repair-steps' (see http://nextcloudappstore.readthedocs.io/en/latest/developer.html#info-xml), which seems the way to go re migration.

As for the typeToStatus, I figure we should remove it and refactor code that is currently using it. It feels the typeToStatus method is a bit of a kludge..

Thanks for working on this! Let me know if you decide to drop it afterall. I will pause my work on this one for now in favour of some other bug(s).

danopz · 2017-07-11T19:28:46Z

Oh sry didn't see you assigned yourself.
Shure I could try to get this done, but also if you want you can still do it yourself, maybe you are more into the codebase.

Indeed True/False are the better option for this.

I will keep working on this until someone stops me from doing this :)

BernhardPosselt · 2017-07-11T19:37:39Z

BTW since you are changing SQL statements it might make sense to throw away the unit tests and replace them with integration tests (examples available in the test folder)

danopz · 2017-07-18T23:16:45Z

I dropped the unit tests which belongs to the changed mappers and added integration tests.
Changed booleans in sql back to integers (0,1) as sqlite does not support true/false. But now it looks like postgres doesn't allow a boolean = integer check.. so we have to use prepared statements, am I right?

Also I added the repair step which checks for previous installed version. Currently I increased the version by a patch level, I think an increase in minor/major would be better?

danopz · 2017-07-20T17:54:29Z

For me it's ready for review.
I don't know why this is failing for MySQL with the message SQLSTATE[42S22]: Column not found: 1054 Unknown column 'items.unread' in 'where clause'" at? NC should have added the new fields?

BernhardPosselt

Good work, everything looks like expected although I didn't have the power to make sure that the code still does the same thing in all places. I don't fully understand the bool casts however.

BernhardPosselt · 2017-07-21T09:13:45Z

lib/Db/Item.php

-        $this->status |= StatusFlag::UNREAD;
+    public function setUnread($value = true) {
+        $this->markFieldUpdated('unread');
+        $this->unread = (bool)$value;


Any reason why we need bool casts?

BernhardPosselt · 2017-07-21T09:30:12Z

I don't know why this is failing for MySQL with the message SQLSTATE[42S22]: Column not found: 1054 Unknown column 'items.unread' in 'where clause'" at? NC should have added the new fields?

I'd take a look at the MySql version on travis. Does it work for you locally on mysql?

BernhardPosselt · 2017-07-21T09:34:03Z

I dropped the unit tests which belongs to the changed mappers and added integration tests.
Changed booleans in sql back to integers (0,1) as sqlite does not support true/false. But now it looks like postgres doesn't allow a boolean = integer check.. so we have to use prepared statements, am I right?

Yep. I'm unsure if the doctrince DAL handles the conversion for you or it could be solved via query builder. Worst case scenario is that we need to create a method that turns bools into whatever format your db currently expects. Check https://github.com/nextcloud/news/blob/master/lib/Db/MapperFactory.php#L23 for how to find out the db

danopz · 2017-07-23T18:30:02Z

Removed the bool casts, they they were there for no real reason
Shame on me for the MySQL error, this was just a copy-paste mistake
For the SQL queries this was already fixed by using the booleans as params, DBAL handles this for us

BernhardPosselt · 2017-08-02T08:14:11Z

Can you take care of the tests?

danopz · 2017-08-02T20:36:42Z

Didn't notice the tests fail again, maybe this was the reason for bool casts. I'll have a look.

danopz · 2017-08-02T21:32:31Z

It's green 😁
I added empty checks to Item::is* methods which shouldn't be to expensive.

BernhardPosselt · 2017-08-03T11:49:24Z

Cool, just a quick question: Can you set up a news install without the pull request, add some feeds and star them, then switch to your branch and ensure that everything still works as expected? Just trying to make sure that everything still works since the PR is quite big.

BernhardPosselt · 2017-08-03T12:16:53Z

From a quick glance over the code the only thing that I don't really like is the usage of empty(). That "function" is pretty tricky and can lead to lots of bugs, see http://php.net/manual/en/function.empty.php

danopz · 2017-08-09T21:24:33Z

From a quick glance over the code the only thing that I don't really like is the usage of empty(). That "function" is pretty tricky and can lead to lots of bugs, see http://php.net/manual/en/function.empty.php

Do you have another idea how to have another way to force booleans? I have in mind that empty checks are faster than bool casts. And without such cast the one unit test was failing. Maybe the problem comes from another place but don't know right now.

danopz · 2017-08-09T23:07:16Z

There still is an issue in the migration where status 6 results in unread=false. I didn't find a proper solution yet..

…ved StatusFlags class + refactor code relying to it

…ite doesn't support true/false

…on repair step

BernhardPosselt · 2017-08-12T09:07:13Z

Do you have another idea how to have another way to force booleans? I have in mind that empty checks are faster than bool casts. And without such cast the one unit test was failing. Maybe the problem comes from another place but don't know right now.

PHP is slow anyways so don't worry about speed of function calls but speed of algorithmic complexity ;) Also better to investigate the source of the issue

There still is an issue in the migration where status 6 results in unread=false. I didn't find a proper solution yet..

There still is an issue in the migration where status 6 results in unread=false. I didn't find a proper solution yet..

Are we sure that all three dbs treat this as truthy?

(status & 4)

I'd expect something like

WHERE (status & 4) = 4  -- starred
WHERE (status & 2) = 2  -- unread

danopz · 2017-08-13T20:09:00Z

Cool, just a quick question: Can you set up a news install without the pull request, add some feeds and star them, then switch to your branch and ensure that everything still works as expected? Just trying to make sure that everything still works since the PR is quite big.

Tested it on MySQL and SQLite - it was working for me.

I'd expect something like...

I dropped usage of the QueryBuilder in favor of a native SQL query in the migration step. Now I'm also using (status & 4) = 4.

PHP is slow anyways so don't worry about speed of function calls but speed of algorithmic complexity ;) Also better to investigate the source of the issue

I will have a deeper look.

danopz · 2017-08-13T22:57:40Z

@BernhardPosselt I changed something in my last commit to remove bool casts and empty checks.

BernhardPosselt · 2017-08-14T08:34:35Z

Ok, reviewed it more closely and lets test it out, as in: merge this and check the effects on our own instances :D

BernhardPosselt · 2017-08-14T08:40:06Z

Seems to have worked fine on postgres, well done :) let's give this more testing before we create a release

danopz changed the title ~~[WIP] Split binary to booleans~~ Split binary to booleans Jul 20, 2017

BernhardPosselt reviewed Jul 21, 2017

View reviewed changes

danopz force-pushed the status-flags branch from 2efc8ff to 191071b Compare August 2, 2017 21:26

danopz added 9 commits August 11, 2017 00:53

replaced old status with 2 flags for unread and starred

76c46cc

add fields to db, replace int(1,0) with booleans in sql queries, remo…

e5adcd7

…ved StatusFlags class + refactor code relying to it

add repair step for migration

193c926

again use integer(1,0) instead of bool in sql queries, because of sql…

0eb2723

…ite doesn't support true/false

add/fix unit tests for new boolean status

f8aa13f

set unread/starred flags as statements in sql

997ac42

fixed mysql unknown column items.unread, fixed marking of read items …

08a9b5c

…on repair step

remove unnecessary bool casts

58937b9

add empty checks to Items::is* methods

216917e

update migration to use native sql instead of the querybuilder

f65378b

danopz force-pushed the status-flags branch from 191071b to f65378b Compare August 13, 2017 15:50

don't cast the flags manually, let the api do the work

9bf795e

BernhardPosselt approved these changes Aug 14, 2017

View reviewed changes

BernhardPosselt merged commit a97dd58 into nextcloud:master Aug 14, 2017

danopz deleted the status-flags branch August 14, 2017 08:51

danopz restored the status-flags branch August 14, 2017 08:54

danopz deleted the status-flags branch March 5, 2018 13:02

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Split binary to booleans #203

Split binary to booleans #203

danopz commented Jul 10, 2017 •

edited

Loading

sonologic commented Jul 11, 2017 via email

sonologic commented Jul 11, 2017

danopz commented Jul 11, 2017

BernhardPosselt commented Jul 11, 2017

danopz commented Jul 18, 2017

danopz commented Jul 20, 2017

BernhardPosselt left a comment

BernhardPosselt Jul 21, 2017

BernhardPosselt commented Jul 21, 2017

BernhardPosselt commented Jul 21, 2017

danopz commented Jul 23, 2017

BernhardPosselt commented Aug 2, 2017

danopz commented Aug 2, 2017

danopz commented Aug 2, 2017

BernhardPosselt commented Aug 3, 2017

BernhardPosselt commented Aug 3, 2017 •

edited

Loading

danopz commented Aug 9, 2017

danopz commented Aug 9, 2017

BernhardPosselt commented Aug 12, 2017 •

edited

Loading

danopz commented Aug 13, 2017

danopz commented Aug 13, 2017

BernhardPosselt commented Aug 14, 2017

BernhardPosselt commented Aug 14, 2017

Split binary to booleans #203

Split binary to booleans #203

Conversation

danopz commented Jul 10, 2017 • edited Loading

Todo

sonologic commented Jul 11, 2017 via email

sonologic commented Jul 11, 2017

danopz commented Jul 11, 2017

BernhardPosselt commented Jul 11, 2017

danopz commented Jul 18, 2017

danopz commented Jul 20, 2017

BernhardPosselt left a comment

Choose a reason for hiding this comment

BernhardPosselt Jul 21, 2017

Choose a reason for hiding this comment

BernhardPosselt commented Jul 21, 2017

BernhardPosselt commented Jul 21, 2017

danopz commented Jul 23, 2017

BernhardPosselt commented Aug 2, 2017

danopz commented Aug 2, 2017

danopz commented Aug 2, 2017

BernhardPosselt commented Aug 3, 2017

BernhardPosselt commented Aug 3, 2017 • edited Loading

danopz commented Aug 9, 2017

danopz commented Aug 9, 2017

BernhardPosselt commented Aug 12, 2017 • edited Loading

danopz commented Aug 13, 2017

danopz commented Aug 13, 2017

BernhardPosselt commented Aug 14, 2017

BernhardPosselt commented Aug 14, 2017

danopz commented Jul 10, 2017 •

edited

Loading

BernhardPosselt commented Aug 3, 2017 •

edited

Loading

BernhardPosselt commented Aug 12, 2017 •

edited

Loading