Editing a struct list variable using pointer not working as expected in go

Question

I have a struct that looks like

type Request struct {
    Name string `json:"name"`
    Parameters []Parameter `json:"parameters"`
}

and

type Parameter struct {
    Attached bool `json:"attached"`
    Script string `json:"script"`
}

Now, I have unmarshalled the json into the struct, and the Script variable has an http location "http://localhost/helloworld.sh". What I am trying to do is, to change the struct variable Parameter.Script from http://localhost/helloworld.sh with the actual content of the script, which is a plain ascii shell script. I wrote a method for the inner struct like

func (p *Parameter) SetScript(script string)  {
    p.Script = script
}

using pointer to Parameter,

and in the GetScript function, try to call that method after getting the response body.

func GetScript(params *Request)  {
  for _, i := range params.Parameters {
    switch i.Attached {
    case false:
        client := new(http.Client)
        req, _ := http.NewRequest("GET", i.Script, nil)
        resp, _ := client.Do(req)
        defer resp.Body.Close()
        reader, _ := ioutil.ReadAll(resp.Body)
        i.SetScript(string(reader))
    }
  }
}

However, when I print the struct after calling this function, it has not modified the variables, and prints the http://localhost/helloworld.sh only. I am able to get the response body, which is the actual content of the script, but I am not able to replace the struct variable from within the GetScript function. Could someone please point out the right way to do this?

Thank you.

Answer 1

The problem is that you are using a for _, i := range loop, and you modify the loop variable inside the loop:

for _, i := range params.Parameters {
    switch i.Attached {
    case false:
        // ...
        i.SetScript(string(reader))
    }
}

The loop variable i is a copy of the slice elements you range over. So if you do any modification to it, you will only modify the copy and not the element in the slice. (Note that it's true that the SetScript() method has a pointer receiver, but it will receive the address of the copy, so it can and will modify only the copy.)

One workaround is use an index-only range, and refer to the slice element using indexing (replace all occurrence of i with params.Parameters[i]):

for i := range params.Parameters {
    switch params.Parameters[i].Attached {
    case false:
        // ...
        params.Parameters[i].SetScript(string(reader))
    }
}

You can simplify the above code by assigning the slice to a local variable (this will copy only the slice header not its elements, and it will refer to the same underlying array), and use if statement instead of that ugly switch:

p := params.Parameters
for i := range p {
    if !p[i].Attached {
        // ...
        p[i].SetScript(string(reader))
    }
}

An alternative simplification / improvement is to take the address of the index expression, and use that (so you can omit repeating it multiple times):

for i := range params.Parameters {
    p := &params.Parameters[i]
    if !p.Attached {
        // ...
        p.SetScript(string(reader))
    }
}