[ISSUE] apierr.GetAPIError does not unmarshal ErrorCode correctly

[nkvuong]

Description
apierr.GetAPIError does not unmarshal ErrorCode correctly

Reproduction
Below is a simplified version of parseErrorFromResponse

func main() {

	myError := apierr.APIError{
		ErrorCode: "RESOURCE_DOES_NOT_EXIST",
		Message:   "Path (/Users/abc) doesn't exist.",
	}
	b, err := json.Marshal(myError)
	if err != nil {
		panic(err)
	}
	print(b)
	var errorBody struct {
		ErrorCode any    `json:"error_code,omitempty"`
		Message   string `json:"message,omitempty"`
	}
	err = json.Unmarshal(b, &errorBody)
	if err != nil {
		panic(err)
	}
	fmt.Printf("%+v\n", errorBody)
}

Expected behavior
errorBody should be populated with ErrorCode & Message, however, ErrorCode is nil in this case

{ErrorCode:<nil> Message:Path (/Users/abc) doesn't exist.}

[renaudhartert-db]

Hi @nkvuong,

apierr.GetAPIError does not unmarshal ErrorCode correctly

Do you have an example of an input/ouput pair for apierr.GetAPIError that produces the incorrect behavior you are referring to?

[nkvuong]

it happened in the failed test in this PR from TF provider - databricks/terraform-provider-databricks#3832. The PR simply replaces the deprecated struct in TF with the Go SDK equivalent.

For example TestResourceUserDelete_NonExistingDir should produce no error due to the error handling in this line
https://github.com/databricks/terraform-provider-databricks/blob/main/scim/resource_user.go#L156. However, apierr.IsMissing is returning false for this test case, because the ErrorCode was not unmarshaled correctly.

Since the only change is replacing the structs, we can simply compare the 2 structs, and the difference is the missing field tags

https://github.com/databricks/terraform-provider-databricks/blob/8d628d198523beb0beb379a94661d31ac8c3b3b7/common/apierr.go#L9-L17

databricks-sdk-go/apierr/errors.go

Lines 30 to 39 in 9bbc945

	// APIError is a generic struct for an api error on databricks
	type APIError struct {
	ErrorCode string
	Message string
	StatusCode int
	Details []ErrorDetail
	// If non-nil, the underlying error that should be returned by calling
	// errors.Unwrap on this error.
	unwrap error
	}

[renaudhartert-db]

I see, thanks!

I think the core of the problem you're facing is that apierr.APIError is not meant to be a substitute for common.APIErrorBody. Rather, it is the "higher level" object that is returned by the SDK service clients, not the SDK HTTP client. Making a clear distinction between these two levels is the reason why the parsing struct was made anonymous in parseErrorFromResponse.

It seems that resource_user.go is not using the SDK service client but the HTTP client directly. Therefore, we cannot use apierr.IsMissing on the returned error given that the HTTP client is not returning an api.APIError to begin with.

I think there's two ways we can resolve this:

• A: Change the terraform code to use the service client instead of the HTTP client — and then truly deprecate common.APIErrorBody.
• B: Implement your own IsMissing function that would take a common.APIErrorBody struct as parameter.

What do you think?

[renaudhartert-db]

Closing this issue as "work as expected"