Details

    • Type: Incident report Incident report
    • Status: Closed
    • Priority: Blocker Blocker
    • Resolution: Fixed
    • Affects Version/s: 2.2.0
    • Fix Version/s: 2.2.2rc1, 2.3.0
    • Component/s: API (A)
    • Labels:

      Description

      have a fresh 2.2.0 install, export zabbix server template, try to import it :

      Row span of screen element located at X - 0 and Y - 2 is too big.

      edit : actually, i might have had an older version of this template present, which would mean that it might have already had that screen with dimensions of 2x2

      maybe we validate xml screen elements against screen data in the db, not xml itself. or something

        Issue Links

          Activity

          Hide
          richlv added a comment - - edited

          in CScreenItem.php validateCreate() the following seems to happen, although i might have gotten it all wrong...

          a) "$screenIds = zbx_objectValues($screenItems, 'screenid');" in my case returns only one id - 4.
          b) then "$dbScreenItems = API::getApi()->select('screens_items',..." for some reason returns items for a whole bunch of ids.
          c) then "$screens = API::getApi()->select('screens', array(..." for some reason returns vsize as 2, although it was 3 up to this moment.

          maybe unrelated things that surprise me...

          1) why does creating a new screen go through the update methods/functions ?
          2) when validating screen, why do we insert some data in the db, then select it only to validate ? is that sane ?

          edit : there might have been an existing template with same name in another group, thus update and db access might be justified. actual problem might be inability to change screen dimensions when updating it during the import

          Show
          richlv added a comment - - edited in CScreenItem.php validateCreate() the following seems to happen, although i might have gotten it all wrong... a) "$screenIds = zbx_objectValues($screenItems, 'screenid');" in my case returns only one id - 4. b) then "$dbScreenItems = API::getApi()->select('screens_items',..." for some reason returns items for a whole bunch of ids. c) then "$screens = API::getApi()->select('screens', array(..." for some reason returns vsize as 2, although it was 3 up to this moment. maybe unrelated things that surprise me... 1) why does creating a new screen go through the update methods/functions ? 2) when validating screen, why do we insert some data in the db, then select it only to validate ? is that sane ? edit : there might have been an existing template with same name in another group, thus update and db access might be justified. actual problem might be inability to change screen dimensions when updating it during the import
          Hide
          Pavels Jelisejevs (Inactive) added a comment -

          Looks like a duplicate of ZBX-6367.

          Show
          Pavels Jelisejevs (Inactive) added a comment - Looks like a duplicate of ZBX-6367 .
          Hide
          richlv added a comment -

          note that span can also be set to 0 in the xml, and then the screen imports properly... not sure whether we want to handle that now

          Show
          richlv added a comment - note that span can also be set to 0 in the xml, and then the screen imports properly... not sure whether we want to handle that now
          Hide
          Eduards Samersovs (Inactive) added a comment - - edited

          Resolved

          • validation before screen item creation
          • deleting screen items which become outside of the screen
          • changing screen item coordinates don't change screen items ids (it was absolutely crazy)
          • screen item span validate become the same as for create and update
          • rowspan and colspan parameters become optional

          Fixed in development branch svn://svn.zabbix.com/branches/dev/ZBX-7338

          Show
          Eduards Samersovs (Inactive) added a comment - - edited Resolved validation before screen item creation deleting screen items which become outside of the screen changing screen item coordinates don't change screen items ids (it was absolutely crazy) screen item span validate become the same as for create and update rowspan and colspan parameters become optional Fixed in development branch svn://svn.zabbix.com/branches/dev/ZBX-7338
          Hide
          Pavels Jelisejevs (Inactive) added a comment - - edited

          (1) Minor correction in r40373.

          Eduards Samersovs OK, CLOSED

          Show
          Pavels Jelisejevs (Inactive) added a comment - - edited (1) Minor correction in r40373. Eduards Samersovs OK, CLOSED
          Hide
          Pavels Jelisejevs (Inactive) added a comment - - edited

          (2) In CScreen::create():

          • Screen should not be created one-by-one, but in bulk;
          • There's no need to use replaceItems() since the new screens don't have any items.

          Eduards Samersovs RESOLVED r.40479

          Pavels Jelisejevs I've made a minor code improvement in r40785. Since DB::insert() preserves the keys in the ID array, we don't need a separate counter. Please review.

          Eduards Samersovs OK, CLOSED

          Show
          Pavels Jelisejevs (Inactive) added a comment - - edited (2) In CScreen::create(): Screen should not be created one-by-one, but in bulk; There's no need to use replaceItems() since the new screens don't have any items. Eduards Samersovs RESOLVED r.40479 Pavels Jelisejevs I've made a minor code improvement in r40785. Since DB::insert() preserves the keys in the ID array, we don't need a separate counter. Please review. Eduards Samersovs OK, CLOSED
          Hide
          Pavels Jelisejevs (Inactive) added a comment - - edited

          (3) In CScreen::replaceItems() most of the code could be replaced with DB::replace().

          Eduards Samersovs As discussed, we need validate input before write to db, so DB::replace() is not proper solution.

          Pavels Jelisejevs CLOSED.

          Show
          Pavels Jelisejevs (Inactive) added a comment - - edited (3) In CScreen::replaceItems() most of the code could be replaced with DB::replace(). Eduards Samersovs As discussed, we need validate input before write to db, so DB::replace() is not proper solution. Pavels Jelisejevs CLOSED.
          Hide
          Pavels Jelisejevs (Inactive) added a comment - - edited

          (4) Deleting screen items which become outside of the screen must also be implemented for template screens. I suggest to implement it in a separate method to avoid cope duplicates.

          Eduards Samersovs RESOLVED r.40496

          Pavels Jelisejevs CLOSED.

          Show
          Pavels Jelisejevs (Inactive) added a comment - - edited (4) Deleting screen items which become outside of the screen must also be implemented for template screens. I suggest to implement it in a separate method to avoid cope duplicates. Eduards Samersovs RESOLVED r.40496 Pavels Jelisejevs CLOSED.
          Hide
          Pavels Jelisejevs (Inactive) added a comment - - edited

          (5) Screen item resourceid parameter should not be required. There's a mistake in the API docs.

          Eduards Samersovs RESOLVED r.40522

          Pavels Jelisejevs CLOSED.

          Show
          Pavels Jelisejevs (Inactive) added a comment - - edited (5) Screen item resourceid parameter should not be required. There's a mistake in the API docs. Eduards Samersovs RESOLVED r.40522 Pavels Jelisejevs CLOSED.
          Hide
          Pavels Jelisejevs (Inactive) added a comment - - edited

          (6) Some problems with checking whether the cell is already taken:

          • The following request successfully creates a new item, although the cell is already taken.
          {
              "jsonrpc": "2.0",
              "method": "screenitem.create",
              "params": {
                  "screenid": 20,
                  "resourcetype": 7,
                  "resourceid": 0
              },
              "id": 9,
              "auth": "c4674d432c610d4fed9d5c13c5f859d8"
          }
          
          • CScreenItem::checkDuplicateResourceInCell() should not be called for each screen item, but for all screen itms at once;
          • Lets change the error message to "Screen %screenName% cell X - %x% Y - %y% is already taken." It's more precise.

          Eduards Samersovs RESOLVED r.40479

          Pavels Jelisejevs CLOSED.

          Show
          Pavels Jelisejevs (Inactive) added a comment - - edited (6) Some problems with checking whether the cell is already taken: The following request successfully creates a new item, although the cell is already taken. { "jsonrpc" : "2.0" , "method" : "screenitem.create" , "params" : { "screenid" : 20, "resourcetype" : 7, "resourceid" : 0 }, "id" : 9, "auth" : "c4674d432c610d4fed9d5c13c5f859d8" } CScreenItem::checkDuplicateResourceInCell() should not be called for each screen item, but for all screen itms at once; Lets change the error message to "Screen %screenName% cell X - %x% Y - %y% is already taken." It's more precise. Eduards Samersovs RESOLVED r.40479 Pavels Jelisejevs CLOSED.
          Hide
          Pavels Jelisejevs (Inactive) added a comment - - edited

          (7) The following request is executed even when the row and column spans are to big:

          {
              "jsonrpc": "2.0",
              "method": "screenitem.create",
              "params": {
                  "screenid": 20,
                  "resourcetype": 7,
                  "resourceid": 0,
                  "rowspan": 1000,
                  "colspan": 1000
              },
              "id": 2,
              "auth": "2c5a20d45c2a808a5a9581bc26f96ca9"
          }
          

          There are a lot of bugs caused by omitting certain parameters. To avoid them you mustn't use isset the check* methods, you always need to make sure that all of the required parameters are present: either from the database, or from default values.

          Eduards Samersovs RESOLVED r.40479

          Pavels Jelisejevs CLOSED.

          Show
          Pavels Jelisejevs (Inactive) added a comment - - edited (7) The following request is executed even when the row and column spans are to big: { "jsonrpc" : "2.0" , "method" : "screenitem.create" , "params" : { "screenid" : 20, "resourcetype" : 7, "resourceid" : 0, "rowspan" : 1000, "colspan" : 1000 }, "id" : 2, "auth" : "2c5a20d45c2a808a5a9581bc26f96ca9" } There are a lot of bugs caused by omitting certain parameters. To avoid them you mustn't use isset the check* methods, you always need to make sure that all of the required parameters are present: either from the database, or from default values. Eduards Samersovs RESOLVED r.40479 Pavels Jelisejevs CLOSED.
          Hide
          Pavels Jelisejevs (Inactive) added a comment - - edited

          (8) When validating the coordinates of the screen item, the error message should reference a specific screen item and screen. Something like:

          The Y coordinate of screen element located at at X - %x% and Y - %y% of screen "%screenName%" is too big.

          Similarly for X.

          Eduards Samersovs RESOLVED r.40479

          Pavels Jelisejevs CLOSED.

          Show
          Pavels Jelisejevs (Inactive) added a comment - - edited (8) When validating the coordinates of the screen item, the error message should reference a specific screen item and screen. Something like: The Y coordinate of screen element located at at X - %x% and Y - %y% of screen "%screenName%" is too big. Similarly for X. Eduards Samersovs RESOLVED r.40479 Pavels Jelisejevs CLOSED.
          Hide
          Pavels Jelisejevs (Inactive) added a comment - - edited

          (9) Trying to save an screen element without a graph triggers an "Incorrect graph ID "0" provided for screen element.", when it should say "No graph ID provided for screen element." or, even better, "Field "Graph name" is mandatory.". Same for map and screen elements.

          Eduards Samersovs RESOLVED r.40520

          Pavels Jelisejevs Some corrections for CScreenItem::checkInput():

          • When loading resourcetype for an updated screen element, it's easier to use CZBXAPI::extendFromObjects() instead of
            			if (isset($screenItem['resourcetype'])) {
            				$resourceType = $screenItem['resourcetype'];
            			}
            			else {
            				$resourceType = $update ? $dbScreenItems[$screenItem['screenitemid']]['resourcetype'] : null;
            			}
            
          • $resourceType cannot be null on line 431
          • Condition "if (!(!isset($screenItem['resourceid']) && $update)) {" seems incorrect. It will never be executed if resourceid is set.
          • don't use empty()
          • when saving a host time clock item without an item, the message says "No host ID provided for screen element." instead of "No item ID ..."
          • In line 508 using zbx_empty() was correct, since 0 is a valid URL.

          Eduards Samersovs RESOLVED r.40765

          Pavels Jelisejevs After you use extendFromObjects() on $screenItems, screen item resourceid cannot be null, so the check in line 430 is not required.

          Eduards Samersovs This check is require in create, as discussed

          Pavels Jelisejevs CLOSED.

          Show
          Pavels Jelisejevs (Inactive) added a comment - - edited (9) Trying to save an screen element without a graph triggers an "Incorrect graph ID "0" provided for screen element.", when it should say "No graph ID provided for screen element." or, even better, "Field "Graph name" is mandatory.". Same for map and screen elements. Eduards Samersovs RESOLVED r.40520 Pavels Jelisejevs Some corrections for CScreenItem::checkInput(): When loading resourcetype for an updated screen element, it's easier to use CZBXAPI::extendFromObjects() instead of if (isset($screenItem['resourcetype'])) { $resourceType = $screenItem['resourcetype']; } else { $resourceType = $update ? $dbScreenItems[$screenItem['screenitemid']]['resourcetype'] : null ; } $resourceType cannot be null on line 431 Condition "if (!(!isset($screenItem ['resourceid'] ) && $update)) {" seems incorrect. It will never be executed if resourceid is set. don't use empty() when saving a host time clock item without an item, the message says "No host ID provided for screen element." instead of "No item ID ..." In line 508 using zbx_empty() was correct, since 0 is a valid URL. Eduards Samersovs RESOLVED r.40765 Pavels Jelisejevs After you use extendFromObjects() on $screenItems, screen item resourceid cannot be null, so the check in line 430 is not required. Eduards Samersovs This check is require in create, as discussed Pavels Jelisejevs CLOSED.
          Hide
          Pavels Jelisejevs (Inactive) added a comment - - edited

          (10) It's possible to create a plain text and simple graph element without an item.

          Eduards Samersovs RESOLVED r.40520

          Pavels Jelisejevs CLOSED.

          Show
          Pavels Jelisejevs (Inactive) added a comment - - edited (10) It's possible to create a plain text and simple graph element without an item. Eduards Samersovs RESOLVED r.40520 Pavels Jelisejevs CLOSED.
          Hide
          Pavels Jelisejevs (Inactive) added a comment - - edited

          (11) Using the API it's possible to create a trigger overview element with a host group that I have no permissions to.

          Eduards Samersovs RESOLVED r.40520

          Pavels Jelisejevs This problem is not resolved.

          Eduards Samersovs RESOLVED r.40765

          Pavels Jelisejevs CLOSED.

          Show
          Pavels Jelisejevs (Inactive) added a comment - - edited (11) Using the API it's possible to create a trigger overview element with a host group that I have no permissions to. Eduards Samersovs RESOLVED r.40520 Pavels Jelisejevs This problem is not resolved. Eduards Samersovs RESOLVED r.40765 Pavels Jelisejevs CLOSED.
          Hide
          Pavels Jelisejevs (Inactive) added a comment - - edited

          (12) Some CScreenItem::checkInput() coding style issues

          Eduards Samersovs RESOLVED r.40479

          Pavels Jelisejevs CLOSED.

          Show
          Pavels Jelisejevs (Inactive) added a comment - - edited (12) Some CScreenItem::checkInput() coding style issues there shouldn't be any new lines between elseif clauses; please format the calls to exception() according to the new guidelines https://www.zabbix.org/wiki/Docs/specs/coding_style#Formatting_3 Eduards Samersovs RESOLVED r.40479 Pavels Jelisejevs CLOSED.
          Hide
          Pavels Jelisejevs (Inactive) added a comment - - edited

          (13) screenitem.update doesn't validate resourceid if resourcetype is not given:

          {
              "jsonrpc": "2.0",
              "method": "screenitem.update",
              "params": {
                  "screenitemid": 84,
                  "resourceid": "3333333"
              },
              "id": 4,
              "auth": "e9d858b5b7fd64e128926a59d8e40e9c"
          }
          

          Similar problem as (7).

          Eduards Samersovs RESOLVED r.40520

          Pavels Jelisejevs Now this request triggers a different error: "The X coordinate of screen element located at X - 0 and Y - 0 of screen \"\" is too big."

          Eduards Samersovs RESOLVED r.40765

          Pavels Jelisejevs CLOSED.

          Show
          Pavels Jelisejevs (Inactive) added a comment - - edited (13) screenitem.update doesn't validate resourceid if resourcetype is not given: { "jsonrpc" : "2.0" , "method" : "screenitem.update" , "params" : { "screenitemid" : 84, "resourceid" : "3333333" }, "id" : 4, "auth" : "e9d858b5b7fd64e128926a59d8e40e9c" } Similar problem as (7). Eduards Samersovs RESOLVED r.40520 Pavels Jelisejevs Now this request triggers a different error: "The X coordinate of screen element located at X - 0 and Y - 0 of screen \"\" is too big." Eduards Samersovs RESOLVED r.40765 Pavels Jelisejevs CLOSED.
          Hide
          Pavels Jelisejevs (Inactive) added a comment - - edited

          (14) screenitem.updatebyposition:

          Eduards Samersovs RESOLVED r.40522

          Pavels Jelisejevs The $found variable is now unused and must be removed.

          Eduards Samersovs RESOLVED r.40765

          Pavels Jelisejevs CLOSED.

          Show
          Pavels Jelisejevs (Inactive) added a comment - - edited (14) screenitem.updatebyposition: it must only accepts an array of screen items, not a single item https://www.zabbix.com/documentation/2.2/manual/api/reference/screenitem/updatebyposition $update should not be an associative array in $update[$dbScreenItem['screenitemid']] = $screenItem; you should't use reset on an associative array in $affectedIds = reset($screenItems); it's better to use "continue 2" instead of "break" together with the $found variable Eduards Samersovs RESOLVED r.40522 Pavels Jelisejevs The $found variable is now unused and must be removed. Eduards Samersovs RESOLVED r.40765 Pavels Jelisejevs CLOSED.
          Hide
          Pavels Jelisejevs (Inactive) added a comment - - edited

          (15) According to the new guidelines, we don't assign variables in if statements (graphs.inc.php:288)

          	if ($dbGraph = DBfetch(DBselect('SELECT g.* FROM graphs g WHERE g.graphid='.zbx_dbstr($graphId)))) {
          		return $dbGraph;
          	}
          

          Eduards Samersovs RESOLVED r.40479

          Pavels Jelisejevs CLOSED.

          Show
          Pavels Jelisejevs (Inactive) added a comment - - edited (15) According to the new guidelines, we don't assign variables in if statements (graphs.inc.php:288) if ($dbGraph = DBfetch(DBselect('SELECT g.* FROM graphs g WHERE g.graphid='.zbx_dbstr($graphId)))) { return $dbGraph; } Eduards Samersovs RESOLVED r.40479 Pavels Jelisejevs CLOSED.
          Hide
          Pavels Jelisejevs (Inactive) added a comment - - edited

          (16) In CScreen::update() please use API::getApi()->select instead of screenitem.get. The same for CScreenItem::validateCreate() and validateUpdate() (permission checks won't work there, it's a separate issue).

          Eduards Samersovs RESOLVED r.40522

          Pavels Jelisejevs This issue is not resolved.

          Eduards Samersovs RESOLVED r.40765

          Pavels Jelisejevs CLOSED.

          Show
          Pavels Jelisejevs (Inactive) added a comment - - edited (16) In CScreen::update() please use API::getApi()->select instead of screenitem.get. The same for CScreenItem::validateCreate() and validateUpdate() (permission checks won't work there, it's a separate issue). Eduards Samersovs RESOLVED r.40522 Pavels Jelisejevs This issue is not resolved. Eduards Samersovs RESOLVED r.40765 Pavels Jelisejevs CLOSED.
          Hide
          Pavels Jelisejevs (Inactive) added a comment - - edited

          (17) Using the API it's possible to change a screen item from a graph to a map with the following request:

          {
              "jsonrpc": "2.0",
              "method": "screenitem.update",
              "params": {
                  "screenitemid": 13,
                  "resourcetype": 2
              },
              "id": 24,
              "auth": "0f06ca623de2fbc956a104c5f40b77ba"
          }
          

          Eduards Samersovs RESOLVED r.40765

          Pavels Jelisejevs CLOSED.

          Show
          Pavels Jelisejevs (Inactive) added a comment - - edited (17) Using the API it's possible to change a screen item from a graph to a map with the following request: { "jsonrpc" : "2.0" , "method" : "screenitem.update" , "params" : { "screenitemid" : 13, "resourcetype" : 2 }, "id" : 24, "auth" : "0f06ca623de2fbc956a104c5f40b77ba" } Eduards Samersovs RESOLVED r.40765 Pavels Jelisejevs CLOSED.
          Hide
          Pavels Jelisejevs (Inactive) added a comment - - edited

          (18) Please remove the "with_templated" option the way we discussed.

          Eduards Samersovs RESOLVED r.40788

          Pavels Jelisejevs CLOSED.

          Show
          Pavels Jelisejevs (Inactive) added a comment - - edited (18) Please remove the "with_templated" option the way we discussed. Eduards Samersovs RESOLVED r.40788 Pavels Jelisejevs CLOSED.
          Hide
          Pavels Jelisejevs (Inactive) added a comment -

          TESTED.

          Show
          Pavels Jelisejevs (Inactive) added a comment - TESTED.
          Hide
          Eduards Samersovs (Inactive) added a comment -

          Fixed in versions 2.3.0 (trunk) r.40830, 2.2.2rc1 r.40827

          Show
          Eduards Samersovs (Inactive) added a comment - Fixed in versions 2.3.0 (trunk) r.40830, 2.2.2rc1 r.40827

            People

            • Assignee:
              Unassigned
              Reporter:
              richlv
            • Votes:
              1 Vote for this issue
              Watchers:
              1 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved: