Standardize GetByName/FindbyName methods

[dataclouder]

NB: This issue is relevant to the implementation of data sources/imports in terraform-provider-vcd.

Problem

Currently we have several methods with the format

func (parent *ParentEntity) GetEntityByName(name string) (Entity, error) {}
func (parent *ParentEntity) FindEntityByName(name string) (Entity, error) {}

We have three problems with these methods:

1. Non standard names. They should all be called Find... or all Get...
2. Some return an Entity structure, some a pointer to the entity
3. Some return an error when the entity is not found, some return nil and an empty structure when the entity was not found.
4. Not really a problem, but a limitation: we only search by name, while we will soon need to search for ID too.

Problem n.2 and n.3 contribute to complicate usage of such methods. Currently we do something like:

    entity, err := parent.FindEntityByName(name)
    if err != nil {
      return err
    }
    if entity == Entity{} {
        return err
    }
    // OR something like
    if entity.Child.HREF == "" {
        return err
    }

The point is that we don't know for sure what the error means, and we don't know what to expect from the return value.

Proposal

To alleviate the problems above, we need to

1. User standard names
2. Make it simpler to detect when the search failed
3. Assign meaning to the errors

Solution 1: remove the error and use pointers

func (parent *ParentEntity) GetEntityByName(name string) *Entity {}

If we want the users to inspect the returned object to determine whether the search was successful, we don't need the error. We just return a pointer to the structure and the receiver function can simply say if entity != nil and that's it.

Solution 2: Give meaning to the error and return pointers

var ErrorEntityNotFound = fmt.Errorf("entity was not found")
// ...
func (parent *ParentEntity) GetEntityByName(name string) (*Entity, error) {
    if weDidNotFindTheEntity {
      return nil, ErrorEntityNotFound
    }
}

Using this method, the receiving function will do:

    entity, err := parent.FindEntityByName(name)
    if err != nil {  // meaning we did not find, or there was some other error
      return someError
    }
    // OR
    if entity == nil { // also this means that we did not find
        return someError
    }
    // OR
    if err ==  ErrorEntityNotFound { // also this means that we did not find
        return someError
    }

Standardization

Whichever solution we choose, we should do six things:

1. Deprecate the existing methods, but leave them in place, as they need to still work in Terraform
2. Create new methods to replace the deprecated ones, with a standard name, such as FetchEntityByNameOrId
3. make the method find the entity using either name or ID
4. the method must return a pointer to the entity structure
5. in case of not found, it will return a nil pointer and if solution n.2 is chosen, a standardized error.
6. For every data source that we implement, we start using the new methods and change usage of the old methods with the new ones for the corresponding resource. When we replace everything, we can safely bump up the version and remove the deprecated things.

I vote for Solution n. 2

CC: @lvirbalas @vbauzysvmware @Didainius

[dataclouder]

We have agreed on solution n.2 above, with the following enhancements:

1. The base name for thee retrieval functions will be Read.
2. There will be a ReadEntityByHref that will search an entity directly
3. ReadEntityByName will only run a parent+direct search by name
4. ReadEntityById will only run a parent+direct search by ID
5. ReadEntityByNameOrId will combine the searches by ID and name.
6. All functions above return (*Entity, error)
7. When the entity is not found, the method must return a non-nil error.
8. The functions in 3, 4, 5 will return an ErrorEntityNotFound when an entity is not found in the parent.
9. Depending on the type of the ParentEntity, the functions in 3, 4, 5 may have an additional refresh parameter that will force a refresh of the parent entity before running the search. The refresh is usually required after creating, modifying, or deleting entities.

Example of implementation

// ReadUserByHref returns a user by its HREF, without need for
// searching in the adminOrg user list
func (adminOrg *AdminOrg) ReadUserByHref(href string) (*OrgUser, error) {
	orgUser := NewUser(adminOrg.client, adminOrg)

	_, err := adminOrg.client.ExecuteRequest(href, http.MethodGet,
		types.MimeAdminUser, "error getting user: %s", nil, orgUser.User)

	if err != nil {
		return nil, err
	}
	return orgUser, nil
}

// ReadUserByName retrieves a user within an admin organization by name
// Returns a valid user if it exists. If it doesn't, returns nil and ErrorEntityNotFound
func (adminOrg *AdminOrg) ReadUserByName(name string, refresh bool) (*OrgUser, error) {
	if refresh {
		err := adminOrg.Refresh()
		if err != nil {
			return nil, err
		}
	}

	for _, user := range adminOrg.AdminOrg.Users.User {
		if user.Name == name {
			return adminOrg.ReadUserByHref(user.HREF)
		}
	}
	return nil, ErrorEntityNotFound
}

// ReadUserById retrieves a user within an admin organization by ID
// Returns a valid user if it exists. If it doesn't, returns nil and ErrorEntityNotFound
func (adminOrg *AdminOrg) ReadUserById(id string, refresh bool) (*OrgUser, error) {
	if refresh {
		err := adminOrg.Refresh()
		if err != nil {
			return nil, err
		}
	}

	for _, user := range adminOrg.AdminOrg.Users.User {
		if user.ID == id {
			return adminOrg.ReadUserByHref(user.HREF)
		}
	}
	return nil, ErrorEntityNotFound
}

// ReadUserByNameOrId retrieves a user within an admin organization
// by either name or ID
// Returns a valid user if it exists. If it doesn't, returns nil and ErrorEntityNotFound
func (adminOrg *AdminOrg) ReadUserByNameOrId(identifier string, refresh bool) (*OrgUser, error) {
	// First look by ID
	orgUser, err := adminOrg.ReadUserByName(identifier, true)
	// if it fails, look by name
	if IsNotFound(err) {
		orgUser, err = adminOrg.ReadUserById(identifier, false)
	}
	return orgUser, err
}