(1) try:

{
 "druleid ": "2",
 "iprange": "192.168.0.1-254",
 "dchecks":  
{"dcheckid": 2}
}

Notice:
Fatal error: Unsupported operand types in /var/www/htmltrunk/frontends/php/include/classes/api/services/CDRule.php on line 340

gunarspujats This API request is incorrect, correct request would be:

{
 "druleid": "2",
 "iprange": "192.168.0.1-254",
 "dchecks":  
 [
   {"dcheckid": 2}
 ]
}

gunarspujats RESOLVED in r57397.

iivs CLOSED

(2)
Added new translation string:

• a numeric value is expected

Removed translation strings:

• Empty input.
• Field "druleid" is required.
• Field "name" is required.
• IP range cannot be empty.
• Incorrect authentication protocol for discovery rule "%1$s".
• Incorrect delay.
• Incorrect privacy protocol for discovery rule "%1$s".
• Incorrect proxyid.
• Incorrect status.

iivs CLOSED

sasha New translation string was added:

• Incorrect value for field "%1$s"

1. this string is incorrect (dot must be at the end of sentence)
2. the error message must be more useful, for example: Incorrect value for field "druleid": an ID is expected. or Incorrect value for field "druleid": a numeric value is expected.

REOPENED

gunarspujats RESOLVED in r58740, changed directly in trunk.

iivs The list matches. Looks correct.
CLOSED

RESOLVED in development branch svn://svn.zabbix.com/branches/dev/ZBX-10049

(3) Before validating range, the ID of drule should be validated first.

{
"druleid": "99922332999231231999223322991231231299922299912319992229931299922333229993"
}

gunarspujats RESOLVED in r58084.

iivs

• Having only ID, I can get errors:
  

      Undefined index: name [discoveryconf.php:63 → CAPIObject->update() → CAPIObject->__call() → czbxrpc::call() → czbxrpc::callAPI() → call_user_func() → CDRule->update() in C:\Development\ZBX-10049\frontends\php\api\classes\CDRule.php:533]
      Undefined index: dchecks [discoveryconf.php:63 → CAPIObject->update() → CAPIObject->__call() → czbxrpc::call() → czbxrpc::callAPI() → call_user_func() → CDRule->update() in C:\Development\ZBX-10049\frontends\php\api\classes\CDRule.php:545]
  

• Can you explain why validateRequiredFields() is after checkInput()?

REOPENED

gunarspujats

Can you explain why validateRequiredFields() is after checkInput()?

No, I can't. Maybe "rudolfs" can, he implemented it in this way in r19332.

gunarspujats RESOLVED in r58136.

iivs CLOSED

(4) IP range now accepts any string for granted with no validation.

{
"druleid": "100100000000046",
"iprange": "192.168.0.1-2514"
}

gunarspujats RESOLVED in r58084.

iivs The IP validation is poor, but then again it's an old branch.

CLOSED

(5) Possible to update without dchecks.

{
"druleid": "100100000000046",
"dchecks": [
            
        ]
}

gunarspujats RESOLVED in r58084.

iivs Since we already changed the behaviour of "dchecks" validation, we might as well fix and old problem that it's possible to add a dcheck like this

{
"druleid": "100100000000046",
"dchecks" : [
            { }
        ]
}

and guess what type does it have.

Undefined index: type [discoveryconf.php:66 → CAPIObject->update() → CAPIObject->__call() → czbxrpc::call() → czbxrpc::callAPI() → call_user_func() → CDRule->update() → CDRule->checkInput() → CDRule->validateDChecks() in C:\Development\ZBX-10049\frontends\php\api\classes\CDRule.php:313]

It's also possible to save any number in "type" field.

REOPENED

gunarspujats RESOLVED in r58136.

iivs While looking at 2.2 I discovered several bugs. Since we're now fixing this for trunk (see (7)), we could do things properly. Here are the things I found:

• Optional fields can be set to null.
• Possible to create two drules with same name.
• Error messages do not specify which drule is faulty.
• Delay max limit is not validated by API (Is validated only by frontend).
• Update queries is executed even if no fields are changed.

Let's see if we can make this work and not repeat same mistakes in trunk.

REOPENED

gunarspujats RESOLVED in r58414.

iivs CLOSED

(6) Coding style:

• Since you started to rewrite the validation function, keep in mind that there arguments should bot be passed by reference. And using a thing called $method is just sad. I would advise to write this one as we normally do with validateCreate() and validateUpdate(). Othewise this code will be merged intro trunk and we don't want this kind of code in trunk. Keep trunk clean and out of old and buggy code.
• L258,274, Please write "elseif" as one word. Otherwise it's just "else" that is missing a curly brace after it. I though the given examples are enough, but apparently not. So I added a special example just for you gunarspujats. See https://zabbix.org/wiki/PHP_coding_guidelines#If.2FElse.2FElseif
• L533,545: NULL must be in lower case. If you want me to update guidelines, I will.
• L253-258: there is an "if" statement inside another "if" statement:

  if (array_key_exists('iprange', $dRule)) {
          if (!validate_ip_range($dRule['iprange'])) {
              self::exception(ZBX_API_ERROR_PARAMETERS, _s('Incorrect IP range "%s".', $dRule['iprange']));
          }
  }
  

• L266,267: When you touch an old code and re-factor it, the use proper parameter checking. Do not use isset() even after rewriting the line. Or don't even touch the code there.
• L267: Remove redundant parenthesis.

gunarspujats Created separate functions validateCreate() and validateUpdate().

RESOLVED in r58235.

iivs The validation for field "name" has changed slightly. It's still possible to get same error when setting it to null. Error says "Discovery rule "" already exists." Both create() and update() methods are affected. Also a bit older bug, but it's possible to set type to null. Instead of in_array() we tend to use SetValidator() to avoid such problems.

REOPENED

gunarspujats RESOLVED in r58417.

iivs

• I don't like the idea of checking range from 1 to 604800 using regular expressions

  /^([1-9]|[0-9]{2,5}|[0-5][0-9]{5}|60[0-3][0-9]{3}|604([0-7][0-9]{2}|[0-8]00))$/

  It's unreadable. And further more 604800 is a define SEC_PER_WEEK and it's should stay that way.

• Why create() method uses partial validator? I think it should use just schema validator. In any case the schema validator is using hardcoded regular expressions instead of defines. I don't think this should've been done this way.

• $dublicate_names and $db_dublicate_names should be properly named.

• Avoid using $this->tableName() and use static strings when using API::getApiService()->select().

• There is no need for $db_dublicate_name = reset($db_dublicate_names);

• API::proxy() should begin with a capital letter.

• $proxies = zbx_objectValues($drules, 'proxy_hostid'); contains only IDs not proxies.

• CDrule.php: 686-688 should be written before 'druleids' => $druleids, because that is another output.

• CDrule.php: 688 the closing bracket should be on a separate line.

• CDrule.php: 504 should be a new line after break;

Please revert all changes regarding schema validator, they are just horrible.

REOPENED

gunarspujats RESOLVED in r58593.

iivs

• It's not dublicate, it's duplicate.
• @param array $db_drules DB discovery rules data.
  there is no such parameter in the function

REOPENED

gunarspujats RESOLVED in r58723.

iivs CLOSED

(7) I moved the code in r58282 to better location so that it's easier to see the final changes with diff. And looking at the changes they seem to grow quite a bit. I mean a lot. Discussed with sasha and decided to fix this only for trunk (and not for 3.0.0, but later for 3.1.0). Please move this and changes to new development branch.

The exists() method is removed for trunk, so be careful when copy pasting code to new development branch. Also in trunk the duplicate name is checked after checkInput(), you might want to look into that too.

gunarspujats RESOLVED in r58346.

iivs API has a poor consistency when it comes to missing parameters. Enter one parameter and it turns out another one is missing. Enter another one and again it turns out that more are missing. That's why it would be better to show what parameters are missing from the beginning. Take ValueMap API, for example, and see a list of missing parameters separated by comma. Few other APIs has this implemented as well. This is the direction we should aim. First check for all missing keys. Otherwise, we could enter API::DRule()->create(['']); and receive many more unnecessary errors. And also in that case we could avoid unreasonable errors like

No "name" given for discovery rule "". [hosts.php:122 → CFrontendApiWrapper->create() → CApiWrapper->__call() → CFrontendApiWrapper->callMethod() → CApiWrapper->callMethod() → CFrontendApiWrapper->callClientMethod() → CLocalApiClient->callMethod() → call_user_func_array() → CDRule->create() → CDRule->validateCreate() → CApiService->checkValidator() → CApiService::exception() in include\classes\api\CApiService.php:923]

Also please notice that the order of validation is off. I have a existing drule. Only after I entered "name", "iprange" and "dchecks" it validated name, but when I entered a different name, it continues to validate "dchecks" parameter. Check the name first for all duplicates, existing names. After that continue validating other parameters.

Similarly with "proxy_hostid" parameter. It checks it once, then checks name for duplicates and then checks ID again. And in the end it gives two different error messages.

Please revert all changes regarding schema validator, they are just horrible.

REOPENED

gunarspujats RESOLVED in r58591.

iivs CLOSED

(8) drule.update:

[
    {
        "druleid": 31,
        "name": "a"
    },
    {
        "druleid": 30,
        "name": "b"
    },
    {
        "druleid": 31,
        "name": "c"
    }
]

Error in query [UPDATE drules SET druleid='31', name='a' WHERE druleid='31'] [Duplicate entry 'a' for key 2]
SQL statement execution has failed "UPDATE drules SET druleid='31', name='a' WHERE druleid='31'". [hosts.php:166 → CFrontendApiWrapper->update() → CApiWrapper->__call() → CFrontendApiWrapper->callMethod() → CApiWrapper->callMethod() → CFrontendApiWrapper->callClientMethod() → CLocalApiClient->callMethod() → call_user_func_array() → CDRule->update() → DB::updateByPk() → DB::update() → DB::exception() in include\classes\db\DB.php:545]

gunarspujats RESOLVED in r58591.

iivs

• Having 'druleid' => false causes:
  

  array_key_exists(): The first argument should be either a string or an integer [hosts.php:129 → CFrontendApiWrapper->update() → CApiWrapper->__call() → CFrontendApiWrapper->callMethod() → CApiWrapper->callMethod() → CFrontendApiWrapper->callClientMethod() → CLocalApiClient->callMethod() → call_user_func_array() → CDRule->update() → CDRule->validateUpdate() → array_key_exists() in include\classes\api\services\CDRule.php:301]

  
  Problem didn't exist before.

• Having 'druleid' => 132132132132 causes
  

  array_flip(): Can only flip STRING and INTEGER values! [hosts.php:129 → CFrontendApiWrapper->update() → CApiWrapper->__call() → CFrontendApiWrapper->callMethod() → CApiWrapper->callMethod() → CFrontendApiWrapper->callClientMethod() → CLocalApiClient->callMethod() → call_user_func_array() → CDRule->update() → CDRule->get() → dbConditionInt() → array_flip() in include\db.inc.php:859]
  Error in query [SELECT dr.druleid,dr.proxy_hostid,dr.name,dr.iprange,dr.delay,dr.nextcheck,dr.status FROM drules dr WHERE ] [You have an error in your SQL syntax; check the manual that corresponds to your MySQL server version for the right syntax to use near '' at line 1]
  array_key_exists(): The first argument should be either a string or an integer [hosts.php:129 → CFrontendApiWrapper->update() → CApiWrapper->__call() → CFrontendApiWrapper->callMethod() → CApiWrapper->callMethod() → CFrontendApiWrapper->callClientMethod() → CLocalApiClient->callMethod() → call_user_func_array() → CDRule->update() → CDRule->validateUpdate() → array_key_exists() in include\classes\api\services\CDRule.php:301]

REOPENED

gunarspujats RESOLVED in r58704.

iivs

• Undefined index: druleid [hosts.php:124 → CFrontendApiWrapper->update() → CApiWrapper->__call() → CFrontendApiWrapper->callMethod() → CApiWrapper->callMethod() → CFrontendApiWrapper->callClientMethod() → CLocalApiClient->callMethod() → call_user_func_array() → CDRule->update() → CDRule->validateUpdate() in include\classes\api\services\CDRule.php:309]
  

• Previously if diven druleid was array, I got a nice error Field "druleid" is required., but now I additionally get more errors than before:
  

  Array to string conversion [hosts.php:127 → CFrontendApiWrapper->update() → CApiWrapper->__call() → CFrontendApiWrapper->callMethod() → CApiWrapper->callMethod() → CFrontendApiWrapper->callClientMethod() → CLocalApiClient->callMethod() → call_user_func_array() → CDRule->update() → CDRule->validateUpdate() → zbx_is_int() → zbx_ctype_digit() → strval() in include\func.inc.php:815]
  preg_match() expects parameter 2 to be string, array given [hosts.php:127 → CFrontendApiWrapper->update() → CApiWrapper->__call() → CFrontendApiWrapper->callMethod() → CApiWrapper->callMethod() → CFrontendApiWrapper->callClientMethod() → CLocalApiClient->callMethod() → call_user_func_array() → CDRule->update() → CDRule->validateUpdate() → zbx_is_int() → preg_match() in include\func.inc.php:857]

REOPENED

gunarspujats RESOLVED in r58723.

iivs I find this construction

elseif (!is_string($drule['druleid']) && !is_int($drule['druleid']) || !zbx_is_int($drule['druleid'])) {
	self::exception(ZBX_API_ERROR_PARAMETERS, _s('Field "%1$s" is not integer.', 'druleid'));
}

terrible.
Please see my changes in r58724

gunarspujats CLOSED

(9) drule.update:

{
    "druleid": "25"
}

updates drule and clears dchecks.

gunarspujats RESOLVED in r58591.

iivs CLOSED

(10) drule.update:
before the fix it was possible to set "proxy_hostid" to "null" and thus resetting the proxy. Now it's possible to pass "null", but it executes successfully without a warning. The correct way to reset the proxy is to allow "0", for update, but forbid "null".

gunarspujats RESOLVED in r58699.

iivs CLOSED

Fixed in:

• pre-3.1.0 (trunk) r58740

(11) Documentation:

• API changes from 3.0 to 3.2

iivs I sligtly changed the sentence, so there are no same words repeating. Please, review.

gunarspujats CLOSED

Fix Version/s:
3.2.0alpha1
What about 3.0.x version? Will it be fixed?