ZABBIX BUGS AND ISSUES
  1. ZABBIX BUGS AND ISSUES
  2. ZBX-8930

Map import successful without elements from map

    Details

    • Type: Incident report Incident report
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 2.4.1
    • Fix Version/s: 2.4.4rc1, 2.5.0
    • Component/s: API (A)
    • Labels:

      Description

      Import map always successful independent of existence map elements. The message is "Imported successfully".

      It happens because of inherited map elements on map, like this:

                     <selement>
                          <elementtype>1</elementtype>
                          <label>test_map</label>
                          <label_location>3</label_location>
                          <x>11</x>
                          <y>180</y>
                          <elementsubtype>0</elementsubtype>
                          <areatype>0</areatype>
                          <width>200</width>
                          <height>200</height>
                          <viewtype>0</viewtype>
                          <use_iconmap>0</use_iconmap>
                          <selementid>2924</selementid>
                          <element>
                              <name>some_map_name</name>
                          </element>
                          <icon_off>
                              <name>Cloud (32)</name>
                          </icon_off>
                          <icon_on/>
                          <icon_disabled/>
                          <icon_maintenance/>
                          <urls/>
                      </selement>
      

        Activity

        Hide
        Pavels Jelisejevs (Inactive) added a comment -

        When trying to import a map that references an unexisting map, we should trigger an error: "Cannot find map "%referencedMap%" used in map "%importedMap%"."

        Show
        Pavels Jelisejevs (Inactive) added a comment - When trying to import a map that references an unexisting map, we should trigger an error: "Cannot find map "%referencedMap%" used in map "%importedMap%"."
        Hide
        Arvids Godjuks (Inactive) added a comment -

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

        Additional info:
        Import was restructured from a do {} while (); until there are no items to iterate over to a 2 step import due to edge case when a map(s) may not be imported due to missing references in all maps that are in the imported file and import reporting a success.
        Now import first adds or updates all maps in the file and with second step handles all the map elements and referencers.

        Show
        Arvids Godjuks (Inactive) added a comment - Fixed in dev branch svn://svn.zabbix.com/branches/dev/ZBX-8930 Additional info: Import was restructured from a do {} while (); until there are no items to iterate over to a 2 step import due to edge case when a map(s) may not be imported due to missing references in all maps that are in the imported file and import reporting a success. Now import first adds or updates all maps in the file and with second step handles all the map elements and referencers.
        Hide
        Arvids Godjuks (Inactive) added a comment - - edited

        (1) No translation changes

        Oleg Egorov CLOSED

        Show
        Arvids Godjuks (Inactive) added a comment - - edited (1) No translation changes Oleg Egorov CLOSED
        Hide
        Pavels Jelisejevs (Inactive) added a comment - - edited

        (2) I've exported a map with a host, deleted it and tried to import it back with the "Create new" checkbox unchecked. The new map shouldn't be created. Instead I get an error:

        Incorrect fields for sysmap. [conf.import.php:190 → CFrontendApiWrapper->import() → CApiWrapper->__call() → CFrontendApiWrapper->callMethod() → CApiWrapper->callMethod() → CFrontendApiWrapper->callClientMethod() → CLocalApiClient->callMethod() → call_user_func_array() → CConfiguration->import() → CConfigurationImport->import() → CConfigurationImport->processMaps() → CMapImporter->import() → CMap->update() → CMap->checkInput() → CApiService::exception() in /opt/lampp/htdocs/zabbix/2.4/frontends/php/include/classes/api/services/CMap.php:366]

        Arvids Godjuks RESOLVED in r50246
        Re-wrote the implementation in a new way, that incorporated the create/update options and also optimized the code

        Pavels Jelisejevs CLOSED.

        Show
        Pavels Jelisejevs (Inactive) added a comment - - edited (2) I've exported a map with a host, deleted it and tried to import it back with the "Create new" checkbox unchecked. The new map shouldn't be created. Instead I get an error: Incorrect fields for sysmap. [conf.import.php:190 → CFrontendApiWrapper->import() → CApiWrapper->__call() → CFrontendApiWrapper->callMethod() → CApiWrapper->callMethod() → CFrontendApiWrapper->callClientMethod() → CLocalApiClient->callMethod() → call_user_func_array() → CConfiguration->import() → CConfigurationImport->import() → CConfigurationImport->processMaps() → CMapImporter->import() → CMap->update() → CMap->checkInput() → CApiService::exception() in /opt/lampp/htdocs/zabbix/2.4/frontends/php/include/classes/api/services/CMap.php:366] Arvids Godjuks RESOLVED in r50246 Re-wrote the implementation in a new way, that incorporated the create/update options and also optimized the code Pavels Jelisejevs CLOSED.
        Hide
        Pavels Jelisejevs (Inactive) added a comment - - edited

        (3) Element updating doesn't respect the "updateExisting" option.

        Arvids Godjuks RESOLVED in r50246

        Pavels Jelisejevs CLOSED.

        Show
        Pavels Jelisejevs (Inactive) added a comment - - edited (3) Element updating doesn't respect the "updateExisting" option. Arvids Godjuks RESOLVED in r50246 Pavels Jelisejevs CLOSED.
        Hide
        Pavels Jelisejevs (Inactive) added a comment - - edited

        (4) Cannot import a map with links between elements.

        Arvids Godjuks RESOLVED in r50246

        Pavels Jelisejevs CLOSED.

        Show
        Pavels Jelisejevs (Inactive) added a comment - - edited (4) Cannot import a map with links between elements. Arvids Godjuks RESOLVED in r50246 Pavels Jelisejevs CLOSED.
        Hide
        Pavels Jelisejevs (Inactive) added a comment - - edited

        (5) Map background images and icon maps must be resolved before creating a map.

        Arvids Godjuks Woun't fix: For images to be resolved and imported you need tick the checkboxes on the "Images" line when selecting what to import.

        Pavels Jelisejevs No, the images checkboxes only affect creating and updating the images in the "images" XML element. Map element backgrounds are map properties and should be imported regardless of the image settings. And the icon maps should be imported as well (although icon map map export is currently broken by ZBX-8950).

        Arvids Godjuks RESOLVED in r50571

        Pavels Jelisejevs Some code issues:

        1. We don't pass parameters by reference, but return a value instead.
        2. CMapImporter::resolveMapImages() is not a good name for a method, since icon maps are not images. May be resolveMapsReferences and resolveMapElementReferences would be better names.
        3. We don't use !empty(), but isset() + boolean checks instead.
        4. The curly brace must be on the same line as the function declaration on line 296.

        Arvids Godjuks RESOLVED in r50639

        Oleg Egorov CLOSED

        Show
        Pavels Jelisejevs (Inactive) added a comment - - edited (5) Map background images and icon maps must be resolved before creating a map. Arvids Godjuks Woun't fix: For images to be resolved and imported you need tick the checkboxes on the "Images" line when selecting what to import. Pavels Jelisejevs No, the images checkboxes only affect creating and updating the images in the "images" XML element. Map element backgrounds are map properties and should be imported regardless of the image settings. And the icon maps should be imported as well (although icon map map export is currently broken by ZBX-8950 ). Arvids Godjuks RESOLVED in r50571 Pavels Jelisejevs Some code issues: We don't pass parameters by reference, but return a value instead. CMapImporter::resolveMapImages() is not a good name for a method, since icon maps are not images. May be resolveMapsReferences and resolveMapElementReferences would be better names. We don't use !empty(), but isset() + boolean checks instead. The curly brace must be on the same line as the function declaration on line 296. Arvids Godjuks RESOLVED in r50639 Oleg Egorov CLOSED
        Hide
        Pavels Jelisejevs (Inactive) added a comment - - edited

        (6) The second update request must update only elements and map links, not all of the map properties.

        Arvids Godjuks RESOLVED in r50246
        Not only it updates only the elements and links, but also does it in a single request (an optimization)

        Pavels Jelisejevs Is there any reason to update the name of the map?

        Arvids Godjuks RESOLVED in r50571 - map name is removed from the second update request

        Oleg Egorov CLOSED

        Show
        Pavels Jelisejevs (Inactive) added a comment - - edited (6) The second update request must update only elements and map links, not all of the map properties. Arvids Godjuks RESOLVED in r50246 Not only it updates only the elements and links, but also does it in a single request (an optimization) Pavels Jelisejevs Is there any reason to update the name of the map? Arvids Godjuks RESOLVED in r50571 - map name is removed from the second update request Oleg Egorov CLOSED
        Hide
        Pavels Jelisejevs (Inactive) added a comment - - edited

        (7) Some coding style issues:

        1. We don't use variable assignment in if statements http://zabbix.org/wiki/PHP_coding_guidelines#Conditional_statements;
        2. In CMapImporter::getMapsWithoutElements() if you use references in where loops, it must always be unset after the loop;
        3. We don't use empty(), use isset() and boolean statements instead.

        Arvids Godjuks RESOLVED in r50246

        Pavels Jelisejevs CLOSED.

        Show
        Pavels Jelisejevs (Inactive) added a comment - - edited (7) Some coding style issues: We don't use variable assignment in if statements http://zabbix.org/wiki/PHP_coding_guidelines#Conditional_statements ; In CMapImporter::getMapsWithoutElements() if you use references in where loops, it must always be unset after the loop; We don't use empty(), use isset() and boolean statements instead. Arvids Godjuks RESOLVED in r50246 Pavels Jelisejevs CLOSED.
        Hide
        Pavels Jelisejevs (Inactive) added a comment - - edited

        (8) The "if (array_key_exists($actionMapItem['name'], $maps)) {" condition seems to be unnecessary in CMapImporter::import().

        And the variable names $mapActionArray and $actionMapItem are very confusing.

        Arvids Godjuks RESOLVED in r50571

        Pavels Jelisejevs CLOSED.

        Show
        Pavels Jelisejevs (Inactive) added a comment - - edited (8) The "if (array_key_exists($actionMapItem ['name'] , $maps)) {" condition seems to be unnecessary in CMapImporter::import(). And the variable names $mapActionArray and $actionMapItem are very confusing. Arvids Godjuks RESOLVED in r50571 Pavels Jelisejevs CLOSED.
        Hide
        Oleg Egorov added a comment - - edited

        (9) Please review my minor changes in r51190, r51678 and r51692

        Ivo Kurzemnieks I made minor coding style and typo fixes. See r51846

        Oleg Egorov CLOSED

        Show
        Oleg Egorov added a comment - - edited (9) Please review my minor changes in r51190, r51678 and r51692 Ivo Kurzemnieks I made minor coding style and typo fixes. See r51846 Oleg Egorov CLOSED
        Hide
        Ivo Kurzemnieks added a comment -

        TESTED,

        but don't forget to close (9) and since dev branch was created from trunk, merge into 2.4

        Show
        Ivo Kurzemnieks added a comment - TESTED, but don't forget to close (9) and since dev branch was created from trunk, merge into 2.4
        Hide
        Oleg Egorov added a comment -

        FIXED IN 2.4.4rc1 r51869 and 2.5.0(trunk) r51870
        CLOSED

        Show
        Oleg Egorov added a comment - FIXED IN 2.4.4rc1 r51869 and 2.5.0(trunk) r51870 CLOSED

          People

          • Assignee:
            Unassigned
            Reporter:
            Alexey Pustovalov
          • Votes:
            0 Vote for this issue
            Watchers:
            3 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved: