[DataAccess] Add WHERE IN .. BETWEEN .. REGEXP to query builder

Review Request #48 — Created Feb. 7, 2013 and submitted

dinos
Lunr
master
lunr
Adds WHERE .. IN, WHERE .. BETWEEN, WHERE .. REGEXP functionality to query builder
Green unit tests
  • 0
  • 0
  • 18
  • 0
  • 18
Description From Last Updated
Felipe
dinos
pprkut
  1. Cool. Nice first step. There are a few things missing though :)
    First, the methods for syntax that is also supported by other DBMS (we check sqlite normally) should be added to the DMLQueryBuilderInterface.
    Second, some of those new methods may need new escaping methods, or work slightly differently. I'm thinking of REGEX here for example.
    WHERE IN may need brackets around the "right" part.
  2. src/Lunr/DataAccess/MySQLDMLQueryBuilder.php (Diff revision 1)
     
     
     
     
     
     
    I prefer the ternary operator for this. Same for the others.
  3. whitespace error :)
  4. 
      
dinos
pprkut
  1. Still no entries in the DMLQueryBuilderInterface
  2. src/Lunr/DataAccess/MySQLDMLQueryBuilder.php (Diff revisions 1 - 2)
     
     
    query_value is a more logical function name, IMHO
  3. src/Lunr/DataAccess/MySQLDMLQueryBuilder.php (Diff revisions 1 - 2)
     
     
    you need to be careful with the escaping. Like this you would create:
    
    WHERE a IN ('SELECT foo FROM bar')
    
    but that would check for all rows where the value of a is actually 'SELECT foo FROM bar' rather than all possible values of the column foo in bar.
  4. src/Lunr/DataAccess/MySQLDMLQueryBuilder.php (Diff revisions 1 - 2)
     
     
    list_value IMHO
  5. src/Lunr/DataAccess/MySQLDMLQueryBuilder.php (Diff revisions 1 - 2)
     
     
    Similar to before, you need to be careful with the escaping. You can't assume all input you get is a string. So beyond creating a list you can't do any special escaping because you know nothing about the values.
    1. I see the mistake here.
      
      Now the question is, where should be this method defined as it's not a escaping method. It seems to me more like a helper to build your query.
    2. That's is correct, but IMHO it's fine where it is. Even though the function doesn't do any escaping, that is also not indicated by the function name. "list_value" would indicate that whatever is returned by the function is a list as string, and the input is an array of items you want to see in that list.
      So, I don't see a reason to move this anywhere else. What do you think? Do you know a better place for it?
    3. No, I don't. I thought that you might have. 
      From the point that these functions are quite simple I would go with you and leave them where they are.
  6. 
      
dinos
pprkut
  1. 
      
  2. probably an oversight that started with me in having_like.
    
    s/WHERE/HAVING/ :)
  3. This should be a String.
    Also make sure that there is only one space between type and value, and value and description
  4. this should be array
  5. You might want additional checks here for the one element and multidimensional cases.
    1. I agree with the one element, but until which depth do you think that should I check multidimensionality? e.g. array(1, 2, array(3, 4, array(5, 6))) is that an input case in this context?
    2. It's an invalid input case that can happen. I'm actually not quite sure how we want to handle this. implode by default throws a Notice when this happens.
      If we want to handle this differently we'd have to go for a foreach instead of an implode.
    3. In the review that I submitted you will find a 1st depth check for multidimensional array. Although it is a proposal I wrote some unit tests for debugging. 
      Also wouldn't be better to sanitize the input in a higher level?
    4. I had a look at the changes and I think you missunderstood me, because it's far too complicated.
      This is not about input sanitizing, as that should indeed happen higher up. This is about "gracefully" handling invalid input, where "gracefully" is defined by us.
      Like I said, implode will throw a Notice in such a case, which is one definition for "gracefully" if that's fine for us. Alternatively it could be something like:
      
      if (is_array($input) === FALSE)
      {
          return '';
      }
      
      foreach($input as &$value)
      {
          if (is_array($value))
          {
              bla;
          }
          else
          {
              foo;
          }
      }
      unset($value);
      
      Getting an invalud input is something you should always account for, not matter what level you're at. The only question is how are we accounting for it.
  6. 
      
dinos
dinos
pprkut
  1. 
      
  2. src/Lunr/DataAccess/DatabaseDMLQueryBuilder.php (Diff revisions 3 - 5)
     
     
     
     
     
     
    another case where I think the ternary op is enough :)
    
    return empty($value) ? '' : '(' . $value . ')';
  3. src/Lunr/DataAccess/DatabaseDMLQueryBuilder.php (Diff revisions 3 - 5)
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
    I don't think this is the best way we can go. IMHO it's very confusing.
    I'd consider a multi-dim array invalid input. SO it really depends on how we want to handle it. If we want to do our own logging => foreach, else a simple implode is enough.
  4. 
      
dinos
pprkut
  1. Ship It!
  2. 
      
dinos
Review request changed

Status: Closed (submitted)

Loading...