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

Feature/support string ids #192

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Feature/support string ids #192

wants to merge 2 commits into from

Conversation

jellehak
Copy link

@jellehak jellehak commented Apr 22, 2021

First of all great library! After wanting to port some data from a mongodb database I stubbled upon that only 'integers' are valid primary keys. It would nice to see 'strings' supported as primary keys in SleekDb or maybe as an options. In this pull request I created a minimal setup to use a string as primary key. Most likely it should be behind some flag to be backward compatible. Hope you can have a look and bring in support for string primary keys. Great work by the way.

@rakibtg
Copy link
Member

rakibtg commented Apr 23, 2021

@jellehak glad you liked SleekDB and for the PR! :)

@Timu57 I think it will break the segmented store feature? What do you think?

@Timu57
Copy link
Member

Timu57 commented Apr 23, 2021

Hi @jellehak thank you for your PR. Every type of contribution is always welcome.

With your implementation I see the following problems:

  1. It will be a breaking change and will not be downwards compatible.
  2. The uniqid function from PHP does not deliver true unique identifiers, so there has to be a check if an id already exist in the store.
  3. The new id as a string is not correctly considered in every method of SleekDB.

@rakibtg I see his/her use case and understand the need for custom id's. We could consider that kind of feature when creating the Schema-Feature. Or we can try to implement it with an configuration of the store, where the user can change the type of the _id from INT AUTOGENERATED to STRING (not auto-generated). We could also implement an auto-generation feature for strings. But then we always have to check if an id already exist in the database.

@jellehak
Copy link
Author

jellehak commented Apr 26, 2021

Thanks for the quick feedback. @Timu57 @rakibtg.

  1. Current state of the pull request should not be merged indeed.
  2. Good point. I just added uniqid as quick starting point ( which only generates unique id's on a single machine ).
  3. At the moment most function work well. Which method you expect problems with?

p.s. @Timu57 Schema-Feature sound nice, but don't rebuild sql :D. Great thing about this library is the ease of use to store any kind of structure. Schema validation should be a very optional feature IMO. Personally I like to do schema validation with 'json-schema' (https://json-schema.org/).

Kind regards,
Jelle

@webghostx
Copy link

webghostx commented May 30, 2021

Another approach would be to be able to use other uniqueKeys in addition to the primaryKey.

It's just an idea without looking into the code. In my current project, in addition to '_id', I also have the key '_hostname' which must be unique.

Currently, I solve it like this within my filter method before updateOrInsert()

// ....
if (isset($entry['_id'])) {
    // update ...(check is missing whether_ id really exists)
    $qb = $this->store->createQueryBuilder();
    $exist = $qb
        ->where(
            ['_hostname', "=", $hostname],
            "AND",
            ['_id', "!=", $entry['_id']]
        )
        ->getQuery()
        ->exists();
    if (!$exist)
        return $hostname;
} else {
    // insert
    $qb = $this->store->createQueryBuilder();
    $exist = $qb
        ->where(
            ['_hostname', "=", $hostname]
        )
        ->getQuery()
        ->exists();
    if (!$exist)
        return $hostname;
}
// else error
return false;
// ....

or is there a better solution?

@Timu57
Copy link
Member

Timu57 commented Jul 11, 2021

Hi @webghostx

It's probably a little bit late, but this here could be a little bit more elegant implementation.

// ....
$where = [['_hostname', "=", $hostname]];

if(isset($entry['_id'])){
  $where[] = ['_id', "!=", $entry['_id']];
}

$exist = $this->store->createQueryBuilder()
  ->where($where)
  ->getQuery()
  ->exists();

return (!$exist) ? $hostname : false;

@webghostx
Copy link

much more elegant - thank you :)

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.

None yet

4 participants