Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with
or
.
Download ZIP

Loading…

Added CUBRID Database 8.4.3+ support. #328

Closed
wants to merge 8 commits into from

7 participants

@kadishmal

PR for #317: Add support for CUBRID Database.

Tested on custom projects. Reverse and Forward engineered demodb demo database which is supplied by CUBRID by default. All successful.

Could not run Propel2's own test suits because of the way these tests are created. Issues with Propel2 test suits are here (CUBRID doesn't provide anything like disabling the foreign keys. Propel2 creates table with no order, i.e. child tables before parent tables, which cause errors) and here (in CUBRID database names must be 17 characters or less. Propel2 uses 18+ characters). Until these issues with test suits are fixed, Propel2 tests can't be run on CUBRID Platform.

Other than that, I have successfully used CUBRID with Propel2.

@cristianoc72
Collaborator

uhmm......it's very difficult to merge some code that breaks test suite, isn't it @willdurand ?

During my current work (Firebird/Interbase drive) I'm encountering the same problems.
The true problem is that Cubrid (and Firebird, too) does not support neither schemas, nor cross-databases queries.
During test:prepare command, when it's building bookstore-schemas, with Postgres and Oracle (which supports schemas) those tables are created inside a different schema; with Mysql (which supports cross-database queries) the same tables are created in a different database, so there is no conflict.
Of course, in an on-the-road test we usually refer to a single database so there's no problem, but to assure a certain quality of our work, we ABSOLUTELY need to privide tests.

So, we should study a workaround to by-pass this test-suite problem. I.e. in test:prepare command, if vendor is "cubrid" or "firebird" we could think to merge all schema xml files in one single file, then process it, to create a single database with all tables for test, without conflict....

Any better ideas?

@willdurand
Owner
@kadishmal

In my opinion, the test suit in Propel2 is designed incorrectly.

A Unit Test means that particular test should test the target component for a specific outcome. AND it also means that this test should setup an environment necessary to execute the test scenario. AND the test environment should be independent for each test, i.e. before the test is run, the database should be empty or prepopulated with only the necessary data necessary for this test scenario. If previous test tests have creates tables, inserted records, everything must be cleaned up before this test is executed.

How it is in Propel2?

A totally separate and independent component sets up a common environment which is used by all tests. Then each unit test simply executes its test scenario.

Any problem with this implementation?

Yes. In case you've forgotten to run that independent script to setup the environment, all your tests are screwed. That what happens with testing CUBRID Database in Propel2: most tests fail, because the environment is not setup. Why is it not? Because it cannot be. Propel2 assumes that all DBMSs support schemas which creates a problem for CUBRID. You can't have two tables with the same name, while if schema was supported, you could. Why this is important? Because only unlike MySQL, CUBRID doesn't support SET foreign_key_checks = 0 which allows to create a child table even if a parent table doesn't exist yet, or drop a parent table even if child tables refer to its PK.

So...

These may seem to be small issues which can be ignored and CUBRID Database support not added to Propel2, but this is not just about CUBRID Database. The code, especially in a software like Propel ORM which provides database abstractions, must not include any vendor specific code in the common base. Everything vendor specific should be in vendor specific components. This includes tests. If some tests need to disable foreign key checks and do some ugly hacking, then it should be isolated. This is why it is important to have tests be independent when the test environment is setup separately for each test and for each database unless there are some common database independent tests.

Personally, I like how test suits are implemented in Hibernate Java Framework and Yii PHP Framework. Each test is completely independent: sets up its own environment, different for each database vendor.

There is always a room for improvement, and these are my thoughts on how you can improve Propel2 at least the test suits.

Let me know if you have comments.

@hhamon
Collaborator

@kadishmal you're completely right. But Propel 2 is based on the legacy Propel 1 codebase, which was built something like 7 years ago. Of course, the tests suite is not completely right.

@kadishmal

Is there any plan to improve it? Otherwise, I'm very sad this much work won't have any chance to get merged.

@willdurand
Owner
@kadishmal

In this case, what we can do/improve to have this PR pass tests or have it merged?

@cristianoc72
Collaborator

I'm working on a solution to run test suite with Cubrid. By now It seems fine. This weekend I'll finish my work and I'll let you know.

@willdurand
Owner

@kadishmal don't worry, this will be added at some point!

@willdurand
Owner

Thanks @cristianoc72 for taking the lead on this ;)

@kadishmal

Thank you @cristianoc72! Let me know if you have something.

@cristianoc72
Collaborator

Current status:
1. Fix CubridPlatform so that CubridPlatformTest and CubridPlatformMigrationTest are green
2. Adjust test:prepare command to correctly build fixtures with --vendor="cubrid"
3. Adjust CubridPlatform and ObjectBuilder to correctly manage CLOB (Cubrid requires an explicit conversion from text to clob via CHAR_TO_CLOB sql function).
4. Fix some bugs in CubridAdapter. Currently, 21% of tests are green.
5. Adjust CubridPlatfrom, CubridAdapter, ModelCriteria and BasePeer to quote each single identifier, inside all queries. I.e. select mytable.position from "mytable" where... doesn't work. It shoul be: select "mytable"."position" from "mytable" where.....
6...n. Everything else coming out later from test suite failures

Work continues........

@kadishmal

Oh, lots of the work done. Thank you @cristianoc72 for the effort!

@cristianoc72
Collaborator

Heya all, my work is almost finished: now test suite builds and runs.
@kadishmal you should merge my "cubrid" branch into your master and review the code once again, before update this PR.

@willdurand
Owner

:star2:

@hhamon
Collaborator

:sunny:

@kadishmal

@cristianoc72, cool! I will do so ASAP.

@kadishmal

I've noticed that some of the tests use ENUM data type. Since it's been implemented in CUBRID 9.0, the ENUM tests will fail with CUBRID 8.4.3. Thus, there are two options:

  1. Remove ENUM test to support CUBRID 8.4.3 as it's the most widely used stable release.
  2. Bump up required CUBRID version to 9.0.

When testing with CUBRID 9.0, ENUM tests were failing. Even though cubrid.schema.xml file defines:

<column name="style" type="ENUM" valueSet="novel, essay, poetry" />

But the generated bookstore.sql schema file has:

CREATE TABLE `book2`
(
    ....
    `style` ENUM,
    ....
);

Obviously this misses the brackets and values list which fails to follow the ENUM syntax rules. Have fixed it in the last commit.

Now, preparing the test environment is successful. Propel correctly creates the database and its schema.

After running phpunit, I have this now:

Tests: 2811, Assertions: 5933, Failures: 6, Errors: 301, Skipped: 39.

I will start checking the errors now.

@willdurand
Owner

Heya guys, so what's the status of this PR? I would go for supporting CUBRID 9.0 only to get ENUM support, but I don't know if 8.4.3 is old/recent, used/not that used. Please, make a choice, rebase, and let's move forward :)

@kadishmal

CUBRID 8.4.3 is the latest stable version released in November 2012. 8.4.x release is the most widely used version as the first 8.4.0 version was released in July 2011. In fact, 8.4.1 is the most widely used version which is fully compatible with 8.4.3.

CUBRID 9.0 (released last October), on the other hand, is the latest beta release for the upcoming 9.1 stable version which should be released in the coming weeks (February). As a beta release 9.0 isn't yet widely adopted, but it's definitely a complete overhaul with 3x performance increase and tons of other new features.

9.1 will obviously be fully compatible with 9.0. So when 9.1 is released I believe many new users will start with 9.1.

However, in my opinion, 8.4.x users will prevail for quite a long time as it's a really stable release proven by time.

So, my suggestion would be to support 8.4.3 release, and skip ENUM related tests for it. If necessary, we can create two separate adapters for these two versions. This is how Hibernate project deals with MySQL 4.x and 5.x branches.

@cristianoc72
Collaborator
@cristianoc72
Collaborator

Hi @kadishmal, which is the current status of this PR?

@kadishmal

Hi @cristianoc72! Sorry for disappearing. I had conferences to speak at so needed some time to get prepared. I will resume and finish this work (Tests: 2811, Assertions: 5933, Failures: 6, Errors: 301, Skipped: 39.) after two weeks.

If someone is willing to check the current working code, I am eager to accept the assistance. If not, I will finish it in May.

@cristianoc72
Collaborator

Good to know! Don't worry, in May it's ok or whenever you have the possibility. Thanks for your work

@kadishmal

I'm back online. Resumed the work on this issue.

@willdurand
Owner

@kadishmal good news :)

@kadishmal

I need someone's clarification here. There are 281 tests which fail because of PDOException: could not find driver. In the tests the code briefly tries to connect to SQLite instead of CUBRID. See the below:

class ArchivableBehaviorObjectBuilderModifierTest extends \PHPUnit_Framework_TestCase
{
    public function setUp()
    {
        ....
        // This test calls buildSchema without any parameter for $dsn, $user, and $pass
        QuickBuilder::buildSchema($schema);
    }
}

class QuickBuilder
{
    ...
    public static function buildSchema($schema, $dsn = null, $user = null, $pass = null, $adapter = null)
    {
        $builder = new self;
        $builder->setSchema($schema);

        return $builder->build($dsn, $user, $pass, $adapter);
    }

    public function build($dsn = null, $user = null, $pass = null, $adapter = null, array $classTargets = null)
    {
        if (null === $dsn) {
            $dsn = 'sqlite::memory:';
        }
        if (null === $adapter) {
            $adapter = new \Propel\Runtime\Adapter\Pdo\SqliteAdapter();
        }
        if (null === $classTargets) {
            $classTargets = $this->classTargets;
        }
        $pdo = new PdoConnection($dsn, $user, $pass);
        ...
    }
    ...
}

When $dsn, $user, $pass are not set in tests, they get assigned with SQLite instance which is obviously wrong.

So, what should I do? Should I skip the entire test for CUBRID (there are many such tests)? Or should I modify tests to pass DSN information to QuickBuilder? How to specify DSN for tests?

@staabm
Collaborator

hmm I think this topic was already discussed in another issue, but I can't remember which one...

WDYT about a global ENV var which can be used to define the "default dsn data" and if it is not defined we fall back to the current defaults (sqlite::memory,..)?

@cristianoc72
Collaborator
@willdurand
Owner

This test suite makes me crazy.. We should state something about it, either skipping tests or add conditional assertions...

@kadishmal

@cristianoc72, I've enabled pdo_sqlite and all tests are green.

OK, but incomplete or skipped tests!
Tests: 2811, Assertions: 6731, Skipped: 40.

CUBRID test database has processed 3759 requests including 12 query errors which are, I suppose, intentionally executed in test scenarios as they have been successfully asserted.

I should agree with @willdurand that Propel2 tests are somewhat unusual. If we still want to maintain them as they are, they need to be thoroughly documented on how to execute them, what to expect when running them against a particular platform, and when to enable pdo_sqlite.

@staabm, I haven't checked but I think the code you've linked simply indicates the environment variables for creating MySQL database and schemas and test:prepare in the bash. I doubt that these environment variables will be used by phpunit. How phpunit will know about how to construct a DSN from the given $DB variable? I don't think it's applicable in phpunit testing. I may be wrong, though. Please correct me if I am.

So, what's next?

@cristianoc72
Collaborator

Fantastic!
Now you should run test suite once again, but against Mysql (I know, it's another strange thing....). Travis runs tests against Mysql, so you should be sure that all tests are green even for Travis.
After that, push to your master and update this PR.

@kadishmal

I will test with MySQL now, but nothing to push, i.e. this PR already includes the latest changes. I haven't changed anything.

@cristianoc72
Collaborator

A push serves to start a new Travis job, which must be green, before merge a PR. @willdurand am I wrong?

@kadishmal

MySQL tests results:

OK, but incomplete or skipped tests!
Tests: 2811, Assertions: 6774, Incomplete: 4, Skipped: 18.

@cristianoc72, right! I forgot to add CUBRID script to travis.yml. I will do now.

@kadishmal
  1. One remark regarding Travis script for CUBRID is that CUBRID doesn't come preinstalled in Travis, unlike MySQL, so our travis.yml should install it before running phpunit.
  2. Does anyone mind if I use Chef to install CUBRID? I maintain cubrid-cookbook which makes it a lot easier to install various versions of CUBRID, its drivers, as well as create any number of databases. We need CUBRID 9.1, the PDO driver, and need to create the test database before test:prepare.

What do you say?

@cristianoc72
Collaborator

Well, it's ok for me. But I see that our travis.yml includes only Mysql scripts, nothing about Postgres or Oracle or Sqlite, which are already supported. Imho, you can leave things as they are, by now.

@kadishmal

Yes, I've noticed that. But I think no tests are run by Travis against PostgreSQL or Oracle. Only MySQL and SQLite at most.

@kadishmal

On my second branch I've tried to modify the travis.yml and add a logic to install CUBRID via the Chef cookbook, create a test database, and install the PDO driver. To my surprise the before_script successfully installs CUBRID, creates a database, but fails to start the test database.

I've reported this issue to Travis (travis-ci/travis-ci#1116) and have found that all Travis Worker VMs support only IPv6. CUBRID doesn't support IPv6 at this moment. Actually, I've never seen any production environment with only IPv6 enabled. Have you? This was the first time.

This means Travis can't run tests agains CUBRID Database. From what @cristianoc72 says, this may be not a big deal as no Propel2 tests are run against PostgreSQL or Oracle.

So, what do we do next?

@kadishmal

I found the solution for CUBRID to run properly on Travis Workers. In the Travis script, as suggested by their team, I point the hostname to 127.0.0.1 IPv4 address, instead of the default IPv6 address. Now CUBRID can start the test database.

Now I'm looking into why PDO can't connect to it. Probably due to the same IPv6 address conflict.

...Generator/Builder/Sql/Cubrid/CubridDataSQLBuilder.php
((1 lines not shown))
+<?php
+
+/**
+ * This file is part of the Propel package.
+ * For the full copyright and license information, please view the LICENSE
+ * file that was distributed with this source code.
+ *
+ * @license MIT License
+ */
+
+namespace Propel\Generator\Builder\Sql\Cubrid;
+
+use Propel\Generator\Builder\Sql\DataSQLBuilder;
+
+/**
+ * MySQL class for building data dump SQL.

Should not be "Cubrid class" ? :smile:

Fixed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
src/Propel/Generator/Platform/CubridPlatform.php
((129 lines not shown))
+
+ $ret .= $this->getDropTableDDL($table);
+ $ret .= $this->getAddTableDDL($table);
+ $ret .= $this->getAddIndicesDDL($table);
+
+ $fks .= $this->getCommentBlockDDL($table->getName() . ' Foreign Key Definition');
+ $fks .= $this->getAddForeignKeysDDL($table);
+ }
+
+ // add foreign key definition at the very end to avoid
+ // "The class 'table_name' referred by the foreign key does not exist" error
+ // since in CUBRID there is no way to turn off foreign keys which is a bad practice anyway
+ $ret .= $fks;
+ $ret .= $this->getEndDDL();
+
+ //echo $ret;

Does this should be included in the code?

@cristianoc72 Collaborator

Fixed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@kadishmal

I've updated the Travis script. Now it installs CUBRID and its PDO driver, and creates a test database. Then runs the same phpunit routine.

CUBRID is installed via CUBRID Chef cookbook. The Chef instructions and other CUBRID installation related code is placed inside the tests directory. I didn't want to pollute the root directory.

The Travis script runs tests both on MySQL and CUBRID in two PHP environments (5.4 and 5.5).

All builds are green: https://travis-ci.org/kadishmal/Propel2/builds/8409870.

What's the next step? Who is picking up the baton?

@cristianoc72
Collaborator

Good news!! Now this PR is ready to be merged.
When they'll have some time, @willdurand and @jaugustin 'll review the code. They'll tell you if this PR 'll have to be rebased or some commits squashed and after that the code'll be merged.

@kadishmal

Nice!

@staabm
Collaborator

Any news here?

@kadishmal

I'm interested, too!

This was referenced
@willdurand
Owner

Can someone rebase this PR so that we can merge it?

@willdurand
Owner

do you, guys, think that we could ship this feature for alpha3?

@cristianoc72
Collaborator
@cristianoc72
Collaborator

Current status:

  • fix merge conflicts (much work!)
  • fix test suite
  • final rebase

Completed!

@cristianoc72
Collaborator

@kadishmal please could you pull my cubrid branch to update this PR?
May be there's something to review in tests/bin/setup.cubrid.sh: could you please get a look at it?

@willdurand willdurand closed this
@marcj
Owner

Why close?

@cristianoc72
Collaborator

I've the last code in my cubrid branch. Just merged pr #527, I'll rebase it once again and I'll submit a new pr.

@willdurand
Owner

cool!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.