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

QueryBuilder API: \MongoRegex instances on string typed identifier fields get discarded #1294

Closed
narcoticfresh opened this issue Dec 3, 2015 · 7 comments
Labels
Milestone

Comments

@narcoticfresh
Copy link
Contributor

Hi guys

There is a proprietary behavior when a user issues an equals() on an identifier field using QueryBuilder as opposed to a 'normal' field (see DocumentPersister).

If I do this:

$builder->field("id")->equals(new \MongoRegex("/^ad.*$/i"));

Then check $builder->getQueryArray() I get:

array (size=1)
  '_id' => string '/^ad.*$/i' (length=9)

As you can see, the MongoRegex instance is lost and the query doesn't work as expected.

I don't know if this is a feature, a bug or wrong usage - so I first created a StackOverflow question detailing this issue:
http://stackoverflow.com/questions/34061566/doctrine-2-odm-querying-id-field-with-mongoregex-like

I'm creating this issue in the hopes to get some answer why doctrine-odm behaves that way as it does or to get an answer how one can do a Regex-based search on an identifier field..

IMHO Doctrine should not intervene in a feature that is possible using MongoDB shell. As all our IDs are strings and not MongoIDs, I see no reason why my MongoRegex gets converted to a plain string. What am I missing?

Thanks so much!

@malarzm malarzm added the Bug label Dec 3, 2015
@malarzm malarzm modified the milestones: 1.0.4, 1.0.x Dec 3, 2015
@malarzm
Copy link
Member

malarzm commented Dec 3, 2015

I agree that ODM should let you do so :) Tagged it as a bug and hopefully I'll get back to it shortly

@narcoticfresh
Copy link
Contributor Author

@malarzm thanks very much; I'll update the StackOverflow question..

I'm not into active development here, is there anything I can help? Would it be a quickfix in DocumentPersister or is it correcting the return of getDatabaseIdentifierValue() and then correct that in each Type?

@malarzm
Copy link
Member

malarzm commented Dec 4, 2015

From top of my head I'd suggest the DocumentPersister and additional check.
If you'd like to work on the fix it'd be great :)
On 4 Dec 2015 10:20, "Dario Nuevo" notifications@github.com wrote:

@malarzm https://github.com/malarzm thanks very much; I'll update the
StackOverflow question..

I'm not into active development here, is there anything I can help? Would
it be a quickfix in DocumentPersister or is it correcting the return of
getDatabaseIdentifierValue() and then correct that in each Type?


Reply to this email directly or view it on GitHub
#1294 (comment)
.

@narcoticfresh
Copy link
Contributor Author

@malarzm
i started on creating the unit test for this bug.. and that yields interesting things..
apparently, this bug only applies to identifier fields of type string - not id or custom_id.

so this bug apparently only appears if:

  • the UUID strategy is used
  • the type is forced to string, meaning that StringType is internally used instead of IdType/CustomidType.

what works

let's consider this annotation class:

/** @ODM\Document */
class GH1294User
{
    /** @ODM\Id(strategy="UUID") */
    public $id;

    /** @ODM\String */
    public $name = false;

this works as expected, as Type::getType($idType)->convertToDatabaseValue($id) goes to the custom_id type - and that type conserves the MongoRegex instance.

what doesn't work

see this annotation (note type="string"):

/** @ODM\Document */
class GH1294User
{
    /** @ODM\Id(strategy="UUID", type="string") */
    public $id;

only now, we see that wrong behavior described here. now, Type::getType($idType)->convertToDatabaseValue($id) goes to StringType which will cast our MongoRegex to string and thus loose it.

now @malarzm, I discovered that

  • in our case, our issue can be resolved by removing type='string' from our XML definitions. this still produces a string value in _id, something that we need. this need was that led us to include type='string' in the first place.

so then:

  • is this still a bug? i think yes.. it shouldn't matter if i force the type to string - I should still be able to Regex search. what you think?
  • is it now maybe a mistake of the handling in StringType - could StringType check for a MongoRegex instance and leave it alone?

i renamed this issue to reflect the issue more precise.

@narcoticfresh narcoticfresh changed the title Why treat equals() specially on identifier fields? (or: How to regex on id fields) \MongoRegex instances on string typed identifier fields get discarded Dec 7, 2015
@narcoticfresh narcoticfresh changed the title \MongoRegex instances on string typed identifier fields get discarded QueryBuilder API: \MongoRegex instances on string typed identifier fields get discarded Dec 7, 2015
@malarzm
Copy link
Member

malarzm commented Dec 7, 2015

@narcoticfresh thank you very much for digging into this!

is this still a bug? i think yes..

I concur :)

could StringType check for a MongoRegex instance and leave it alone?

I think so, I wouldn't expect anyone to use MongoRegex outside of search context for string fields. For what it's worth I started a branch that would prepare all values used in query builder (or at least all values which field's type could be found) and this will surely save us from the issue there.

narcoticfresh added a commit to narcoticfresh/mongodb-odm that referenced this issue Dec 7, 2015
@narcoticfresh
Copy link
Contributor Author

For what it's worth I started a branch that would prepare all values used in query builder

@malarzm did u push that said branch? i don't really get what you're saying (how you prepare those values)..

i created a failing unit test that should be green when the bug is ironed out..

@malarzm
Copy link
Member

malarzm commented Dec 7, 2015

i don't really get what you're saying (how you prepare those values)..

Given this query:

$var = '7'; // ofc $var came from somewhere else ;)
$qb->field('integer')->equals($var)

will not yield any results because driver will use string in query and that will not match any document in database. Obviously it's simple to cast such variables values to int by hand but also it's something that ODM could do for you in most common cases as it knows the type of field you're querying. Also it gets more interesting for custom types that requires some conversion as in #1179.

did u push that said branch?

Unfortunately no, I started working on that last week but afterwards went to Paris and haven't gotten back to that yet.

narcoticfresh added a commit to narcoticfresh/mongodb-odm that referenced this issue Dec 13, 2015
@malarzm malarzm modified the milestones: 1.0.4, 1.0.x Dec 14, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants