[BUG] We need to validate the existence of the file and correct data structure of the token file

[fernando-villalba]

Description

We need to validate the existence of the file and correct data structure of the token file provided by the user with credentials_file

It looks like currently we are not validating the data or the existence of the credentials_file provided by the user.

Acceptance Criteria

• Ensure that we check for the existence of the file
• Validity of the file
• If the file is too large, over 20 megs or something like that, perhaps we should fail too even before trying to read the file to avoid issues. Not sure how this is handled right now --> This is a bonus and could be done as other ticket for all files in general.

[Praveen005]

Hi @uzaxirr,

For this issue, would the following approach suffice?

func readTokenFromFile(path string) (string, error) {
    // check if the file exists
    _, err := os.Stat(path)
    if errors.Is(err, fs.ErrNotExist) {
        return "", fmt.Errorf("file does not exist: %s", path)
    }

    // Check file size: 20 MB limit
    if err = utils.CheckFileSize(path, 20*1024*1024); err != nil {
        return "", err
    }

    // read the file
    data, err := os.ReadFile(path)
    if err != nil {
        return "", fmt.Errorf("error reading file (%s): %v", path, err)
    }

    var config struct {
        APIKeys map[string]string `json:"apikeys"`
        Meta    struct {
            CurrentAPIKey string `json:"current_apikey"`
        } `json:"meta"`
    }

    if err := json.Unmarshal(data, &config); err != nil {
        return "", fmt.Errorf("failed to parse JSON from %s: %v. Please ensure the input JSON is correctly formatted and all required fields are present", path, err)
    }

    if config.APIKeys == nil || config.Meta.CurrentAPIKey == "" {
        return "", fmt.Errorf("invalid structure in %s, missing required fields", path)
    }

    currentKeyName := config.Meta.CurrentAPIKey

    token, ok := config.APIKeys[currentKeyName]
    if !ok {
        return "", fmt.Errorf("API key '%s' not found in %s", currentKeyName, path)
    }
    return token, nil
}

I have question, should I return these errors to users, or just log it like below?

log.Printf("[ERROR] error reading token from CLI config: %v\n", err)

And if we are to return these errors, what to do in the following scenario?

If I encountered an error while reading from credentials_file and we fall back to ~/.civo.json but got one error here as well. Should we club these two errors and show both?

[fernando-villalba]

I would read and handle the error directly instead, no need for os.stat

    data, err := os.ReadFile(path)
    if err != nil {
        if errors.Is(err, fs.ErrNotExist) {
            return "", fmt.Errorf("file does not exist: %s", path)
        }
        return "", fmt.Errorf("error reading file (%s): %w", path, err)
    }

Also use %w for returning original function errors, in case they ever need to be unwrapped.

Aside from that, as part of the error message unmarshalling, I would output a sample to the user of how it should look like, so they can copy and paste that for use. Maybe something like this:

exampleJSON := `
{
    "apikeys": {
        "tf_key": "token_here"
    },
    "meta": {
        "current_apikey": "tf_key"
    }
}`

    if err := json.Unmarshal(data, &config); err != nil {
        return "", fmt.Errorf("failed to parse JSON from %s: %w. Please ensure the input JSON file is correctly formatted and all required fields are present. Expected format:\n%s", path, err, exampleJSON)
    }

    if config.APIKeys == nil || config.Meta.CurrentAPIKey == "" {
        return "", fmt.Errorf("invalid structure in %s, missing required fields. Expected format:\n%s", path, exampleJSON)
    }

    currentKeyName := config.Meta.CurrentAPIKey

    token, ok := config.APIKeys[currentKeyName]
    if !ok {
        return "", fmt.Errorf("API key '%s' not found in %s", currentKeyName, path)
    }
    return token, nil

Please note this is just sample code to give you ideas, I haven't double checked it or refined it.

[Praveen005]

Hi Sir,

In the current state we look for token in the following order:

Token attribute -> CIVO_TOKEN env. variable -> credentials_file attribute -> ~/.civo.json

If an error occurs upstream, we don't return immediately; instead, we fall back to the next option. Finally, we return after trying all options. This approach in a way enhances the user experience by ensuring that even if one stage fails, the token may still be read correctly.

Now, my question is, should I club the errors? say I got an error while reading from credentials_file and again from ~/.civo.json should I show both the errors to the user?

or

Return as and when you get an error without falling back downstream?

or

Should we keep the error handling in its current state?

[Praveen005]

Hi @uzaxirr, I'm currently stuck on the error handling part. When you have some time, could you please review it?

[uzaxirr]

hiii @Praveen005 you should return as and when you get an error without falling back downstream. Suppose a user have 2 accounts and one of them is configured via CLI but for TF they may wanna use a diff one. But mistakenly provided the wrong token value. So in this case it make sence to return the error immediately without falling to downstream.

[Praveen005]

That's an interesting edge case @uzaxirr. I will make the changes. Thank you. Should I go ahead and raise a PR?

[uzaxirr]

That's an interesting edge case @uzaxirr. I will make the changes. Thank you. Should I go ahead and raise a PR?

yes please do