Better gRPC error handling

[cmaglie]

This is a proposal to uniform gRPC error handling.

For now only the upload command has been ported, as a proof-of-concept. It will be extended to all the arduino-cli code base if approved.

This PR, once fully implemented, should:

• change all gRPC API implementations to return a status.Status instead of an error (the Status message is how the gRPC transports errors on RPC calls).
• application-specific errors must be mapped to gRPC codes
• application-specific errors that may be useful to "identify" client-side are reported also in the Status.details field (for example the UploadRequiresProgrammerError)

[silvanocerza]

I like this, it's a great improvement that's very much needed. I just have some doubt about the returning an rpc.UploadResponse even when the status.Status might contain errors.

[per1234]

Is this considered a breaking change to the API?

If so, does the standard process for documenting it need to be done still?
https://arduino.github.io/arduino-cli/latest/CONTRIBUTING/#pull-request-checklist

[cmaglie]

🤦🏼 right, we should prepare a migration guide.

I'll write down some notes later today...

[silvanocerza]

Shouldn't we fix this by handling this err instead of ignoring it?

[per1234]

Probably already clear, but I did this because this assignment of err was ignored and the error type from the declaration was incompatible with the change of lib.LibrarySearch()'s return type from error to *status.Status.

If it's to be ignored, I think it's best to make that explicit by using _. But if an error return from ParseLibraryReferenceArg() is significant in this context, then of course it's better to handle it.

[cmaglie]

Yep, the err was ignored also before this PR (it was overwritten without being checked). This should be fixed in another PR BTW.

[silvanocerza]

The translation files must be updated, the rest is good. I'll approve as soon that's fixed. 👍

[cmaglie]

After trying the current solution for a bit I found two annoying problems:

1. the error messages are not consistent, just to make an example the snippet:

   	packageManager := i.PackageManager
   	if packageManager == nil {
   		return nil, status.New(codes.InvalidArgument, tr("Invalid instance"))
   	}

   is repeated a lot in the codebase, but there are cases where the message is different:

   		return nil, errors.Errorf(tr("incorrect instance")
   		return nil, errors.Errorf(tr("unable to find an instance with ID: %d"), instanceID)

2. developers that imports the commands package directly will have the following code broken:

   detailsResp, err := board.Details(context.Background(), detailsReq)
   if err != nil {
       fmt.Println("Error getting board details:", err)
       return
   }

   even if it seems perfectly reasonable it prints an address instead of an error message, because *status.Status did not
   implement a String() method, this is not idiomatic.

PROPOSAL

To address the two problems above, while rebasing this PR on master, I'd like to try a different approach:

1. to avoid repetition of error strings we define a bunch of common errors objects in commands/errors.go, something like:

   type InvalidInstanceError struct{}
   
   func (e *InvalidInstanceError) Error() string {
          return tr("Invalid instance")
   }

2. to make life easier for developers importing the commands package I'll try to:

   • set back the return type to error instead of *status.Status.
   • define a CommandError interface:

     type CommandError interface {
            ToRPCStatus() *status.Status
     }

   • implement the CommandError interface on all errors returned from the commands public API, in this case the InvalidInstanceError show above will become:

     type InvalidInstanceError struct{}
     
     func (e *InvalidInstanceError) Error() string {
            return tr("Invalid instance")
     }
     func (e *InvalidInstanceError) ToRPCStatus() *status.Status {
            return status.New(codes.InvalidArgument, e.Error())
     }

   • the wrappers in deamon.go will use an helper function to convert the error in the correct status.Status:

     func convertError(err error) error {
            if rpcErr, ok := err.(CommandError); ok {
                    return rpcErr.ToRPCStatus().Err()
             }
            return err
     }

[silvanocerza]

Yes, I 100% agree with this new design. I think it also make it easier to handle errors internally and recover them gracefully.
Let's go! 🐎

Requested changes have been made. Thanks!

[silvanocerza]

All in all it's a great improvement but there are some small things to fix.