Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fixes [Bug]: Incorrect connection creation #4 #13

Open
wants to merge 17 commits into
base: main
Choose a base branch
from

Conversation

ineersa
Copy link
Contributor

@ineersa ineersa commented Jul 10, 2024

PR #5 got messed up after merges.
Fixed issues, tested that it's working.

@@ -30,33 +31,6 @@ public function sync(): void
$this->db->sync();
}

public function getDb(): LibSQL
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@darkterminal As we were talking #5 (comment)

I don't think it's good idea to have this methods, because they are ambiguous.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would it be better if this were reserved for internal derivatives only?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can already access $this->db, and even tho, since it's actually public it's not reserved for internal usage only, maybe it would be better to set it to protected.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Make sense... the property are need to be protected.

return $this->db->getConnectionMode();
}

public function statement($query, $bindings = []): bool
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why are you removing actual implementation?

    /**
     * Execute an SQL statement and return the boolean result.
     *
     * @param  string  $query
     * @param  array  $bindings
     * @return bool
     */
    public function statement($query, $bindings = [])
    {
        return $this->run($query, $bindings, function ($query, $bindings) {
            if ($this->pretending()) {
                return true;
            }

            $statement = $this->getPdo()->prepare($query);

            $this->bindValues($statement, $this->prepareBindings($bindings));

            $this->recordsHaveBeenModified();

            return $statement->execute();
        });
    }

This will totally mess up things further, especially if you will work with models inserts/updates.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I had to lie with this to the Connection class:

public function getDb(): LibSQL
{
    return $this->db->getDb();
}

public function getConnectionMode(): string
{
    return $this->db->getConnectionMode();
}

public function statement($query, $bindings = []): bool
{
    $res = $this->select($query, $bindings);

    return !empty($res);
}

public function getPdo(): LibSQLDatabase
{
    return $this->db;
}

public function getReadPdo(): LibSQLDatabase
{
    return $this->db;
}

public function select($query, $bindings = [], $useReadPdo = true)
{
    $result = (array) parent::select($query, $bindings, $useReadPdo);

    $resultArray = array_map(function ($item) {
        return (array) $item;
    }, $result);

    return $resultArray;
}

Because when I used this method:

    /**
     * Execute an SQL statement and return the boolean result.
     *
     * @param  string  $query
     * @param  array  $bindings
     * @return bool
     */
    public function statement($query, $bindings = [])
    {
        return $this->run($query, $bindings, function ($query, $bindings) {
            if ($this->pretending()) {
                return true;
            }

            $statement = $this->getPdo()->prepare($query);

            $this->bindValues($statement, $this->prepareBindings($bindings));

            $this->recordsHaveBeenModified();

            return $statement->execute();
        });
    }

Database connections will use the PDO engine to perform binding, whereas libSQL itself does not have natural binding parameters like PDO.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Database connections will use the PDO engine to perform binding, whereas libSQL itself does not have natural binding parameters like PDO.

Yes, that's right, and you are rewriting it inside your Statement class, method execute.

On my dev branch I've changed structure a little, to make concerns for statement/connection classes more clear.
main...ineersa:turso-driver-laravel:dev-branch

Primary I done that to address issue with lastInsertId which was not working and was implemented in the wrong place, and it's actually crucial for Eloquent models.

Also I have some strange problems with bindings on INSERT currently, where TEXT column data is truncated.

But if you will try and look into my dev branch, you can see that everything is working via base methods.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yea problem is more fundamental, and lies in internal not compatibility between this implementation and PDO classes.
#14 this one has roots from there also, I will try to look at it on the next week

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So, now it's clearer why I made a lot of unconventional things with this package.

And for reason about the connection string need to follow the Turso conventional, because the binary itself is have a logic to identifying the connection mode like this:

pub fn get_mode(
    url: Option<String>,
    auth_token: Option<String>,
    sync_url: Option<String>,
) -> String {
    match (url, auth_token, sync_url) {
        (Some(ref url), Some(ref auth_token), Some(ref sync_url))
            if (url.starts_with("file:") || url.ends_with(".db") || url.starts_with("libsql:"))
                && !auth_token.is_empty()
                && (sync_url.starts_with("libsql://")
                    || sync_url.starts_with("http://")
                    || sync_url.starts_with("https://")) =>
        {
            "remote_replica".to_string()
        }
        (Some(ref url), Some(ref auth_token), _)
            if !auth_token.is_empty() && url.starts_with("libsql://")
                || url.starts_with("http://")
                || url.starts_with("https://") =>
        {
            "remote".to_string()
        }
        (Some(ref url), _, _)
            if url.starts_with("file:")
                || url.ends_with(".db")
                || url.starts_with("libsql:")
                || url.contains(":memory:") =>
        {
            "local".to_string()
        }
        _ => "Mode is not available!".to_string(),
    }
}

Check if is Remote Replica Connection?

  • url starts with file:, ends with .db, or starts with libsql:.
  • auth_token is not empty.
  • sync_url starts with libsql://, http://, or https://.

Check if is Remote Connection?

  • auth_token is not empty.
  • url starts with libsql://, http://, or https://.

Check if is Local Connection?

  • url starts with file:, ends with .db, starts with libsql:, or contains :memory:.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yea problem is more fundamental, and lies in internal not compatibility between this implementation and PDO classes.

Please remember, the real challenge is to make it compatible without using PDO. So far what I've done is freestyle which may look very strange.

src/Database/LibSQLConnection.php Outdated Show resolved Hide resolved
@@ -88,16 +69,6 @@ public function useDefaultSchemaGrammar()
}
}

public function createReadPdo(array $config): ?LibSQLDatabase
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Again, createReadPdo() is used only in ConnectionFactory, which we are not using, and it's ConnectionFactory method, not connection.

src/Database/LibSQLConnection.php Outdated Show resolved Hide resolved
src/Database/LibSQLDatabase.php Show resolved Hide resolved
@darkterminal
Copy link
Collaborator

Hi @ineersa, I just made a major change to libSQLConnection to change all database transaction related operations that used PDO to now use LibSQL and I kept the LibSQLConnectionFactory to register the new driver database.

You can try it and I would like to know how it turned out from your side.

@ineersa
Copy link
Contributor Author

ineersa commented Jul 17, 2024

Hi @ineersa, I just made a major change to libSQLConnection to change all database transaction related operations that used PDO to now use LibSQL and I kept the LibSQLConnectionFactory to register the new driver database.

You can try it and I would like to know how it turned out from your side.

@darkterminal
I've moved part which adds testing into separate PR - https://github.com/tursodatabase/turso-driver-laravel/pull/15/files#diff-0755941eb5e1efaa30f08d0ea030c92b47b4d4974e93967e9d446e9f3e7e22d9R26

Some of the connection methods are working now on main like https://github.com/tursodatabase/turso-driver-laravel/pull/15/files#diff-0755941eb5e1efaa30f08d0ea030c92b47b4d4974e93967e9d446e9f3e7e22d9R59 local file for example

Some of them are not - https://github.com/tursodatabase/turso-driver-laravel/pull/15/files#diff-0755941eb5e1efaa30f08d0ea030c92b47b4d4974e93967e9d446e9f3e7e22d9R26 like memory for example.

I've tried to follow README.md, so if you tested your changes and they were working, please update documentation with correct setup for each connection type and add tests for those connection types.

This PR has also some tests in this regard but they are using default Laravel way of database connection definition - https://github.com/tursodatabase/turso-driver-laravel/pull/13/files#diff-0755941eb5e1efaa30f08d0ea030c92b47b4d4974e93967e9d446e9f3e7e22d9

@darkterminal
Copy link
Collaborator

darkterminal commented Jul 17, 2024

@ineersa I will update the README follow the tests that I already merged. test-feature are passed!

image

But, the escapeString and quote in LibSQLConnection is still error in the test:

image

@tonysm
Copy link

tonysm commented Oct 21, 2024

Folks, any chance of getting this merged? I'm working on an app that deals with multiple connections (multiple libsql connections but also non-libsql connections), so I think this patch is on the right direction, right?

@ineersa
Copy link
Contributor Author

ineersa commented Oct 21, 2024

Folks, any chance of getting this merged? I'm working on an app that deals with multiple connections (multiple libsql connections but also non-libsql connections), so I think this patch is on the right direction, right?

I doubt, there was too much changes at time of this PR, so it would be better to revisit it with current driver version.
Root causes of problems with multiple connections was fixed here yes, but there was too much things not working, so i gave up and went to SQLite. Might get back to this when will have more time.

@tonysm
Copy link

tonysm commented Oct 21, 2024

@ineersa Thanks for the heads up and for the work on this one so far! Being limited to a single connection is a bummer. Not sure how to overcome that for the app I'm working on. But thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants