API Error Handling with Invalid Attributes

[Kurt]

If I pass in a "where" filter for an attribute that does not exist (for example, a misspelling like "nmae" instead of "name") the API seems to return rows as if I passed no filters in. IMO the API should throw an error if you try to filter on a non-existent attribute. Returning rows as if there was no filter is risky as well, as if the misspelling/bug goes uncaught your code could be operating on entities it's not supposed to.

[yuri]

It's a controversial point. Will require a lot of testing. Since sometimes there could be passed not existing seach params handled separately for a not standard behaviour.

[Kurt]

I can't imagine a use case where people would run a search against fields that don't exist. Maybe I am misunderstanding though, as you are infinitely more familiar with this product than I am.

[bandtank]

Kurt I agree with you. An invalid search parameter should either return zero results or an error. I can't think of a use case where it would be useful to see results when the query was invalid in the first place.

[yuri]

It's already used. I think we can live with this.

Returning an empty list is a bad idea. Throwing an error would be more proper.

[bandtank]

yuri Why is returning an empty list bad? The client should be checking the length of the array before trying to process it. It's very unusual to return results from an invalid query. An empty array makes the most sense for that use case because the error status is only supposed to be used when the call had an error, not the results. An empty array isn't an error, but a bad field should be an error.

This is happening with internal objects as well and it's very confusing. For example,

75    $approver = $this->getEntityManager()->getRepository('User')->where(array(
76       'initials' => $approvedBy
77     ))->findOne();
78
79     if(!$approver) {
80       fwrite($hw,"Incorrect number of approvers (initials = '$approvedBy')\n");
81       return false;
82     } 

This doesn't work as intended because

$approver 

is set to a totally unexpected user due to the fact that

$approvedBy 

is null in this case. The first result returned is pretty much random. I spent hours trying to figure out what was wrong before I realized the repository class is trying to return something even if no results are found. The result in this case must be empty or null for it to make any sense because every user has non-null initials

In a similar situation, I used 'where initial => $approvedBy' instead of 'initials' and the result was not empty. That should have thrown an exception because 'initial' isn't a field in my entity. Instead, I got a random row as a result. To fix this, you have to inspect each result to see if it makes any sense, which defeats the whole purpose of the where query. I should be able to rely on my database calls to return the data I asked for instead of double checking all of the results.

I ended up needing to do this to every call:

84     if($approvedBy != $approver->get('initials')) {
85       fwrite($hw,"Wrong approver returned: ");
86       return false;
87     } 

I should never get a result that fails that check because the where clause is supposed to make it impossible, but it happens often.

[yuri]

It needs a lot of testing.

You can try to patch it for your instance:

https://github.com/espocrm/espocrm/b.../Base.php#L776

if (!isset($entity->fields[$field])) {
    $whereParts[] = '0'; // add this line
    continue;
} 

[yuri]

This fix brought about many issues in many places.