[Kde-pim] Review Request 119610: Relations support

Tue Aug 5 10:47:17 BST 2014

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/119610/#review63837
-----------------------------------------------------------

Awesome! Conceptually this is absolutely perfect, just some small issues in the code.

server/src/handler/fetchscope.cpp
<https://git.reviewboard.kde.org/r/119610/#comment44488>

    Wrong indentation

server/src/handler/relationfetch.cpp
<https://git.reviewboard.kde.org/r/119610/#comment44489>

    I think you are missing code to actually parse the "left" or "right" arguments from the command

server/src/handler/relationfetch.cpp
<https://git.reviewboard.kde.org/r/119610/#comment44494>

    Unless you are planning to support GID and RID-based fetch, you should throw exception when mScope.scope() != Uid

server/src/handler/relationfetch.cpp
<https://git.reviewboard.kde.org/r/119610/#comment44492>

    You can use ImapStreamParser::readUtf8String() here - then resource will be s QString and you won't have to convert it below.

server/src/handler/relationfetch.cpp
<https://git.reviewboard.kde.org/r/119610/#comment44490>

    Use ImapStreamParser::readUtf8String() here too - then "types" can be QStringList and you won't have to convert it from BA when you want to pass it to QueryBuilder

server/src/handler/relationfetch.cpp
<https://git.reviewboard.kde.org/r/119610/#comment44491>

    This should use Query::In, because otherwise you are creating query

    SELECT * FROM Relations LEFT JOIN ... WHERE left = 1 AND typeName = 'Type1' AND typeName = 'Type2'

    which can obviously never yield any results. Instead you want something like

    if (types.count() == 1) {
       WHERE left = 1 AND typeName = 'Type1'
     } else if (types.count() > 1) {
       WHERE left = 1 AND typeName IN ('Type1', 'Type2')

    (there seems to be a problem with "IN" that only has one value in the list, hence the split).

    Other question is: do we actually need to support fetch of multiple relation types at once?

server/src/handler/relationfetch.cpp
<https://git.reviewboard.kde.org/r/119610/#comment44496>

    Should remoteID be internal to resources?

server/src/handler/relationremove.cpp
<https://git.reviewboard.kde.org/r/119610/#comment44497>

    readUtf8String()

server/src/handler/relationremove.cpp
<https://git.reviewboard.kde.org/r/119610/#comment44498>

    I think resources should be able to delete relations by their Remote ID. You can constraint this operation to be allowed only from resource context.

server/src/handler/relationstore.cpp
<https://git.reviewboard.kde.org/r/119610/#comment44500>

    readUtf8String()

server/src/handler/relationstore.cpp
<https://git.reviewboard.kde.org/r/119610/#comment44499>

    I think this should be permitted only in resource context. Also, readUtf8String() :)

server/src/handler/relationstore.cpp
<https://git.reviewboard.kde.org/r/119610/#comment44501>

    I think we could spare ourselves of this entire "existing relation" query just by having a UNIQUE leftId,rightId,typeId index on the Relation table. Let database do the heavy-lifting - that's what they are for :)

server/src/handler/relationstore.cpp
<https://git.reviewboard.kde.org/r/119610/#comment44502>

    Move this check right after the itemsQuery.result(), then, instead of the Q_FOREACH above, you can do simply do

    if (items[0].collection().resourceId() != items[1].collection().resourceId()) {
      throw ....
     }

server/src/handler/relationstore.cpp
<https://git.reviewboard.kde.org/r/119610/#comment44503>

    This notification should go before the items notification one. It makes more sense to first announce a new stuff, before we try to use it. 
    Additionally, it might allow for some optimizations on the client side (Monitor will already have the relation and items cached when itemsRelationsChanged arrives).

server/src/storage/akonadidb.xml
<https://git.reviewboard.kde.org/r/119610/#comment44495>

    I understand databases as much as the next guy, but doesn't this index optimize for `WHERE leftId = X AND rightId = Y` lookups? Wouldn't having per-column indexes be better, since in most cases we will be doing lookup based on only one side (??) ?

    As mentioned in the RELATIONSTORE handler, I think we should have a UNIQUE leftId,rightId,typeId index too.

server/src/storage/notificationcollector.h
<https://git.reviewboard.kde.org/r/119610/#comment44504>

    Bad indentation I think (or maybe it's just messed up in reviewboard?)

server/src/storage/notificationcollector.h
<https://git.reviewboard.kde.org/r/119610/#comment44505>

    No more spaces in parentheses \o/

server/src/storage/notificationcollector.cpp
<https://git.reviewboard.kde.org/r/119610/#comment44506>

    Should be indented by 4 spaces

server/src/storage/notificationcollector.cpp
<https://git.reviewboard.kde.org/r/119610/#comment44507>

    Same here

server/tests/unittest/relationhandlertest.cpp
<https://git.reviewboard.kde.org/r/119610/#comment44508>

    Q_ASSERT in tests sucks - when it fails the test just aborts and the /tmp/akonadiserver-test env is not cleaned up (and sysadmins don't like when we leave mess in /tmp on Jenkins). Just print qWarning() and return an empty list - you can QCOMPARE(list.size(), 1) in the test method itself.

    We could try to intercept SIG{TERM,SEGV,...} somewhere in AkTest and do an "emergency cleanup", but for now let's just avoid asserts.

server/tests/unittest/relationhandlertest.cpp
<https://git.reviewboard.kde.org/r/119610/#comment44511>

    testStoreRelation maybe?

server/tests/unittest/relationhandlertest.cpp
<https://git.reviewboard.kde.org/r/119610/#comment44509>

    Could you QCOMPARE individual parts of the releations here? In case of failure it's easier to spot what exact values don't match, and it does not rely on (potentially broken) comparision operator of the object you are actually testing

server/tests/unittest/relationhandlertest.cpp
<https://git.reviewboard.kde.org/r/119610/#comment44514>

    You are missing a scenario with multiple types specified. That would reveal that the RELATIONFETCH handler is actually buggy ;-)

server/tests/unittest/relationhandlertest.cpp
<https://git.reviewboard.kde.org/r/119610/#comment44517>

    Let's have at least one LEFT- and one RIGHT-based fetch test.

- Dan Vrátil

On Aug. 5, 2014, 11:19 a.m., Christian Mollekopf wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/119610/
> -----------------------------------------------------------
> 
> (Updated Aug. 5, 2014, 11:19 a.m.)
> 
> 
> Review request for Akonadi.
> 
> 
> Repository: akonadi
> 
> 
> Description
> -------
> 
> Relations support
> 
> 
> Diffs
> -----
> 
>   server/src/handler/relationstore.h PRE-CREATION 
>   server/src/handler/relationstore.cpp PRE-CREATION 
>   server/src/notificationsource.cpp eaa40f8cd566629398a592def5ab548c5a924793 
>   server/src/storage/akonadidb.xml 4ab487bba74a492673b440cada69db691d38a4f3 
>   server/src/storage/datastore.cpp d34af2852a108ddb4b1e6fedd880bf0807e9f7b3 
>   server/src/storage/notificationcollector.h 8819b2f22c5ae45c01122c64ceb40d240ea81538 
>   server/src/storage/notificationcollector.cpp f9f7936c39a99d890534d83f0ef55fdc7a47510e 
>   server/tests/unittest/CMakeLists.txt b9744d93a3b0cb9e895141c10ddaf2703f12acf0 
>   server/tests/unittest/dbinitializer.h PRE-CREATION 
>   server/tests/unittest/dbinitializer.cpp PRE-CREATION 
>   server/tests/unittest/relationhandlertest.cpp PRE-CREATION 
>   server/src/handler/relationremove.h PRE-CREATION 
>   server/src/handler/relationremove.cpp PRE-CREATION 
>   server/src/handler/fetchscope.h 1e2a9dc69b7f30930864fad4ca98632c1a28b5fc 
>   server/src/handler/fetchscope.cpp ea6108e80053c6470aec0ffdc8998265c059b87f 
>   server/src/handler/relationfetch.h PRE-CREATION 
>   server/src/handler/relationfetch.cpp PRE-CREATION 
>   libs/notificationmessagev2.cpp b8ddc2442af2d2c773d67fca32cb21c6cd65ab71 
>   libs/notificationmessagev2_p.h 4727007b8b80c4495daba873c43ec538d4ebf4d5 
>   libs/notificationmessagev2_p_p.h 2d200abcc31cd9ed8a2e8f711a6dbed24a500c3d 
>   libs/notificationmessagev3.cpp 82b35fa4b63c951487ba631e22141dce13d013f5 
>   libs/protocol_p.h 2ec2a2e27edbd350cb9fad2320b916ef125da952 
>   server/CMakeLists.txt e4829f3b0f5e42c70b2c6a80e23825d749ea5573 
>   server/src/handler.cpp e72451a0fcac7e2528a3a66f1ea7e81fc59cbccf 
>   server/src/handler/fetchhelper.h d002f28e409f7a3902c03438afdd9453f989f51e 
>   server/src/handler/fetchhelper.cpp 84a58c7b4ed8f98e7fd89b3b1ec4a86f0db85d6f 
> 
> Diff: https://git.reviewboard.kde.org/r/119610/diff/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Christian Mollekopf
> 
>

_______________________________________________
KDE PIM mailing list kde-pim at kde.org
https://mail.kde.org/mailman/listinfo/kde-pim
KDE PIM home page at http://pim.kde.org/