Merge pull request #672 from yast/use-element-path

Use ElementPath class

.rubocop.yml

@@ -56,6 +56,9 @@ Metrics/AbcSize:
 # ExcludedMethods: refine
 Metrics/BlockLength:
   Max: 500
+  # RSpec is known as DSL with big blocks
+  Exclude:
+    - test/**/*
 
 # Offense count: 64
 # Configuration parameters: CountBlocks.

package/autoyast2.changes

@@ -1,3 +1,9 @@
+-------------------------------------------------------------------
+Wed Aug 26 10:31:54 UTC 2020 - Imobach Gonzalez Sosa <igonzalezsosa@suse.com>
+
+- Unify profile element paths (bsc#1175680).
+- 4.3.40
+
 -------------------------------------------------------------------
 Wed Aug 26 08:17:59 UTC 2020 - Michal Filka <mfilka@suse.com>
 

package/autoyast2.spec

@@ -22,7 +22,7 @@
 %endif
 
 Name:           autoyast2
-Version:        4.3.39
+Version:        4.3.40
 Release:        0
 Summary:        YaST2 - Automated Installation
 License:        GPL-2.0-only
@@ -41,8 +41,8 @@ BuildRequires:  libxml2-tools
 # xsltproc for AutoinstClass
 BuildRequires:  libxslt
 BuildRequires:  rubygem(%{rb_default_ruby_abi}:rspec)
-# AutoYaST issue handling
-BuildRequires:  yast2 >= 4.3.2
+# AutoYaST ElementPath class
+BuildRequires:  yast2 >= 4.3.20
 # FileSystems.read_default_subvol_from_target
 BuildRequires:  yast2-xml
 BuildRequires:  yast2-transfer
@@ -66,8 +66,8 @@ BuildRequires:  rubygem(%rb_default_ruby_abi:yast-rake)
 
 Requires:       autoyast2-installation = %{version}
 Requires:       libxslt
-# XML.validate
-Requires:       yast2 >= 4.3.8
+# AutoYaST ElementPath class
+Requires:       yast2 >= 4.3.20
 Requires:       yast2-core
 Requires:       yast2-country >= 3.1.13
 # Moving security module to first installation stage

src/include/autoinstall/ask.rb

@@ -20,19 +20,6 @@ def initialize_autoinstall_ask(_include_target)
       Yast.import "UI"
     end
 
-    def path2pos(pa)
-      pos = []
-      Builtins.foreach(Builtins.splitstring(pa, ",")) do |p|
-        if Builtins.regexpmatch(p, "^[1,2,3,4,5,6,7,8,9,0]+$")
-          index = Builtins.tointeger(p)
-          pos = Builtins.add(pos, index)
-        else
-          pos = Builtins.add(pos, p)
-        end
-      end
-      deep_copy(pos)
-    end
-
     def createWidget(widget, _frametitle)
       widget = deep_copy(widget)
       ret = Left(widget)
@@ -423,20 +410,14 @@ def askDialog
 
                 # Save the value in the profile (and also as default value)
                 Ops.set(ask, "default", val)
-                pos = path2pos(Ops.get_string(ask, "path", ""))
                 if Ops.get_string(ask, "path", "") != "" # Set value in the profile
-                  Profile.current = Profile.setElementByList(
-                    pos,
-                    val,
-                    Profile.current
+                  Profile.current = Profile.set_element_by_path(
+                    ask["path"], val, Profile.current
                   )
                 end
                 Builtins.foreach(Ops.get_list(ask, "pathlist", [])) do |p|
-                  pos2 = path2pos(p)
-                  Profile.current = Profile.setElementByList(
-                    pos2,
-                    val,
-                    Profile.current
+                  Profile.current = Profile.set_element_by_path(
+                    p, val, Profile.current
                   )
                 end
 

src/modules/Profile.rb

@@ -8,6 +8,7 @@
 require "yast2/popup"
 
 require "autoinstall/entries/registry"
+require "installation/autoinst_profile/element_path"
 
 module Yast
   class ProfileClass < Module
@@ -609,54 +610,62 @@ def ReadXML(file)
       false
     end
 
-    def setMValue(l, v, m)
-      l = deep_copy(l)
-      v = deep_copy(v)
-      m = deep_copy(m)
-      i = Ops.get_string(l, 0, "")
-      tmp = Builtins.remove(l, 0)
-      if Ops.greater_than(Builtins.size(tmp), 0)
-        if Ops.is_string?(Ops.get(tmp, 0))
-          Ops.set(m, i, setMValue(tmp, v, Ops.get_map(m, i, {})))
-        else
-          Ops.set(m, i, setLValue(tmp, v, Ops.get_list(m, i, [])))
-        end
-      else
-        Builtins.y2debug("setting %1 to %2", i, v)
-        Ops.set(m, i, v)
-      end
-      deep_copy(m)
-    end
-
-    def setLValue(l, v, m)
-      l = deep_copy(l)
-      v = deep_copy(v)
-      m = deep_copy(m)
-      i = Ops.get_integer(l, 0, 0)
-      tmp = Builtins.remove(l, 0)
-      if Ops.greater_than(Builtins.size(tmp), 0)
-        if Ops.is_string?(Ops.get(tmp, 0))
-          Ops.set(m, i, setMValue(tmp, v, Ops.get_map(m, i, {})))
+    # Returns a profile merging the given value into the specified path
+    #
+    # The path can be a String or a Installation::AutoinstProfile::ElementPath
+    # object. Although the real work is performed by {setElementByList}, it is
+    # usually preferred to use this method as it takes care of handling the
+    # path.
+    #
+    # @example Set a value using a XPath-like path
+    #   path = "//a/b"
+    #   set_element_by_path(path, 1, {}) #=> { "a" => { "b" => 1 } }
+    #
+    # @example Set a value using an ask-list style path
+    #   path = "users,0,username"
+    #   set_element_by_path(path, "root", {}) #=> { "users" => [{"username" => "root"}] }
+    #
+    # @param path [ElementPath,String] Profile path or string representing the path
+    # @param value [Object] Value to write
+    # @param profile [Hash] Initial profile
+    # @return [Hash] Modified profile
+    def set_element_by_path(path, value, profile)
+      profile_path =
+        if path.is_a?(::String)
+          ::Installation::AutoinstProfile::ElementPath.from_string(path)
         else
-          Ops.set(m, i, setLValue(tmp, v, Ops.get_list(m, i, [])))
+          path
         end
-      else
-        Builtins.y2debug("setting %1 to %2", i, v)
-        Ops.set(m, i, v)
-      end
-      deep_copy(m)
+      setElementByList(profile_path.to_a, value, profile)
     end
 
-    #  this function is a replacement for this code:
+    # Returns a profile merging the given value into the specified path
+    #
+    # The given profile is not modified.
+    #
+    # This method is a replacement for this YCP code:
     #      list<any> l = [ "key1",0,"key3" ];
     #      m[ l ] = v;
-    #  @return [Hash]
-    def setElementByList(l, v, m)
-      l = deep_copy(l)
-      v = deep_copy(v)
-      m = deep_copy(m)
-      m = setMValue(l, v, m)
-      deep_copy(m)
+    #
+    # @example Set a value
+    #   path = ["a", "b"]
+    #   setElementByList(path, 1, {}) #=> { "a" => { "b" => 1 } }
+    #
+    # @example Add an element to an array
+    #   path = ["users", 0, "username"]
+    #   setElementByList(path, "root", {}) #=> { "users" => [{"username" => "root"}] }
+    #
+    # @example Beware of the gaps!
+    #   path = ["users", 1, "username"]
+    #   setElementByList(path, "root", {}) #=> { "users" => [nil, {"username" => "root"}] }
+    #
+    # @param path [Array<String,Integer>] Element's path
+    # @param value [Object] Value to write
+    # @param profile [Hash] Initial profile
+    # @return [Hash] Modified profile
+    def setElementByList(path, value, profile)
+      profile = deep_copy(profile)
+      merge_element_by_list(path, value, profile)
     end
 
     # @deprecated Unused, removed
@@ -777,6 +786,22 @@ def edit_profile(modules = nil, target: :default)
         end
       end
     end
+
+    # @see setElementByList
+    def merge_element_by_list(path, value, profile)
+      current, *remaining_path = path
+      current_value =
+        if remaining_path.empty?
+          value
+        elsif remaining_path.first.is_a?(::String)
+          merge_element_by_list(remaining_path, value, profile[current] || {})
+        else
+          merge_element_by_list(remaining_path, value, profile[current] || [])
+        end
+      log.debug("Setting #{current} to #{current_value.inspect}")
+      profile[current] = current_value
+      profile
+    end
   end
 
   Profile = ProfileClass.new

test/profile_test.rb

@@ -603,4 +603,90 @@ def patterns_list
       expect(subject.SaveSingleSections("/tmp")).to eq({})
     end
   end
+
+  describe "#set_element_by_path" do
+    let(:profile) { double("profile") }
+    let(:value) { double("value") }
+    let(:new_profile) { double("new_profile") }
+
+    context "when a string is given as path" do
+      it "sets the element by using the path's parts" do
+        expect(subject).to receive(:setElementByList).with(
+          ["users", 0, "username"], value, profile
+        ).and_return(new_profile)
+        result = subject.set_element_by_path("users,0,username", value, profile)
+        expect(result).to eq(new_profile)
+      end
+    end
+
+    context "when a profile path object is given as path" do
+      let(:path) { Installation::AutoinstProfile::ElementPath.from_string("groups,0,name") }
+      it "sets the element by using the path's parts" do
+        expect(subject).to receive(:setElementByList).with(
+          ["groups", 0, "name"], value, profile
+        ).and_return(new_profile)
+        result = subject.set_element_by_path(path, value, profile)
+        expect(result).to eq(new_profile)
+      end
+    end
+  end
+
+  describe "#setElementByList" do
+    let(:profile) do
+      {
+        "users" => [
+          { "username" => "root" },
+          { "username" => "guest" }
+        ]
+      }
+    end
+    let(:path) { ["users", 1, "username"] }
+
+    context "when the element exists" do
+      it "replaces its value" do
+        new_profile = subject.setElementByList(path, "admin", profile)
+        expect(new_profile["users"][1]).to eq(
+          "username" => "admin"
+        )
+      end
+    end
+
+    context "when the element does not exist" do
+      let(:path) { ["users", 1, "realname"] }
+
+      it "adds the element in the given path" do
+        new_profile = subject.setElementByList(path, "Guest User", profile)
+        expect(new_profile["users"][1]).to eq(
+          "username" => "guest", "realname" => "Guest User"
+        )
+      end
+    end
+
+    context "when the element is supposed to be an array member but it does not exist" do
+      let(:path) { ["users", 3, "username"] }
+
+      it "adds an element to the array" do
+        new_profile = subject.setElementByList(path, "admin", profile)
+        expect(new_profile["users"][3]).to eq(
+          "username" => "admin"
+        )
+      end
+
+      it "fills any gap with nil" do
+        new_profile = subject.setElementByList(path, "admin", profile)
+        expect(new_profile["users"][2]).to be_nil
+      end
+    end
+
+    context "when parent elements are missing" do
+      let(:path) { ["groups", 0, "name"] }
+
+      it "adds all the full hierarchy up to the given path" do
+        new_profile = subject.setElementByList(path, "root", profile)
+        expect(new_profile["groups"]).to eq(
+          [{ "name" => "root" }]
+        )
+      end
+    end
+  end
 end