Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

adds error handling support #1100

Merged
merged 36 commits into from
Feb 10, 2022
Merged

adds error handling support #1100

merged 36 commits into from
Feb 10, 2022

Conversation

baywet
Copy link
Member

@baywet baywet commented Feb 1, 2022

Summary

fixes #411

Adds error handling to kiota clients (more details on the logic being applied in the markdown document part of this PR).

I've already logged Error conversion adjustments in the conversion library to fix the last quirks (mostly hardcoded names conflicting with languages naming conventions).

All the examples below result in the following output

Could not find a property named 'displayName2' on type 'microsoft.graph.message'.

Selecting displayName2 is intentional to have the service return an error.

CSharp usage

try {
     var mailFoldersResult = await client.Users["vincent@biret365.onmicrosoft.com"].MailFolders["Inbox"].GetAsync(q => {
        q.Select = new string[] { "id", "displayName2"};
     });
 } catch (Error ex) {
    Console.WriteLine(ex.Error_prop.Message);
}

Go usage

client := u.NewApiClient(adapter)
options := &m.MessagesRequestBuilderGetOptions{
    Q: &m.MessagesRequestBuilderGetQueryParameters{
        Select: []string{"id", "displayName2"},
    },
}
response, err := client.UsersById("vincent@biret365.onmicrosoft.com").Messages().Get(options)
if err != nil {
    casted, ok := err.(*sdkerr.Error)
    if ok {
        fmt.Print(*((*casted.GetError()).GetMessage()))
    } else {
        fmt.Printf("Error getting messages: %v\n", err)
    }
    return
}

Java usage

try {
    var mailFoldersResult = client.users("vincent@biret365.onmicrosoft.com").mailFolders("Inbox").get(q -> {
        q.select = new String[] { "id", "displayName2"};
    }).get();
    System.out.println("Hello world!" + messagesResult.getValue().size() + mailFoldersResult.getDisplayName());
} catch (java.util.concurrent.ExecutionException ex) {
    Throwable innerEx = ex.getCause();
    if(innerEx instanceof graphjavav4.utilities.models.odata.Error) {
       graphjavav4.utilities.models.odata.Error error = (graphjavav4.utilities.models.odata.Error) innerEx;
       System.out.println(error.getError().getMessage());   
    }
}

TypeScript usage

const client = new ApiClient(requestAdapter);
try {
  const messagesResult = await client.usersById("vincent@biret365.onmicrosoft.com").mailFoldersById("Inbox").get({
  select: ["displayName2", "id"]
  });
  console.log(`messages Folder display name ${messagesResult?.displayName}`);
} catch(err) {
   console.log(`Error: ${(err as Error_escaped).error_escaped?.message}`);
}

Todo list

(this is for me, and also for other languages owners to understand how this is implemented)

TODO CSharp

  • update code-gen to pass the error mapping in executor methods to request adapter
  • update request adapter interface to accept the error mapping
  • update request adapter implementation to use error mapping during response handling
  • use the new refiners in the language refiner + unit tests
  • bump versions for abstractions and http packages
  • code-gen unit tests
  • update the response handler interface to accept the error mapping
  • add intermediate exception type in abstraction and have generated types derive from that instead

TODO TypeScript

  • update code-gen to pass the error mapping in executor methods to request adapter
  • update request adapter interface to accept the error mapping
  • use the new refiners in the language refiner + unit tests
  • update request adapter implementation to use error mapping during response handling
  • bump versions for abstractions and http packages
  • code-gen unit tests
  • update the response handler interface to accept the error mapping
  • add intermediate exception type in abstraction and have generated types derive from that instead

TODO Java

  • update code-gen to pass the error mapping in executor methods to request adapter
  • use the new refiners in the language refiner + unit tests
  • update request adapter interface to accept the error mapping
  • update request adapter implementation to use error mapping during response handling
  • bump versions for abstractions and http packages
  • code-gen unit tests
  • update the response handler interface to accept the error mapping
  • add intermediate exception type in abstraction and have generated types derive from that instead

TODO Go

  • update code-gen to pass the error mapping in executor methods to request adapter
  • use the new refiners in the language refiner + unit tests
  • update request adapter interface to accept the error mapping
  • update request adapter implementation to use error mapping during response handling
  • bump versions for abstractions and http packages
  • code-gen unit tests
  • update the response handler interface to accept the error mapping
  • add intermediate exception type in abstraction and have generated types derive from that instead

TODO global

  • refiner to inherit from exception for classes marked as error
  • refiner that adds a reference to error types in class usings
  • refiner to add usings for exception type when a class is error
  • ping PHP team to reflect changes
  • list that change in ruby catch up
  • ping python team to reflect changes
  • add changelog entry
  • update conceptual docs

Generation diff

microsoft/kiota-samples#498

Description being used

microsoftgraph/msgraph-sdk-powershell#1060

@baywet baywet added this to the TypeWriter Replacement milestone Feb 1, 2022
@baywet baywet added generator Issues or improvements relater to generation capabilities. Go TypeScript Pull requests that update Javascript code Csharp Pull requests that update .net code Java labels Feb 1, 2022
@baywet baywet self-assigned this Feb 1, 2022
@baywet baywet force-pushed the feature/error-description branch 2 times, most recently from 6971010 to 64a4c93 Compare February 2, 2022 16:35
@baywet baywet force-pushed the feature/error-description branch from 88f40c5 to ade2fd1 Compare February 3, 2022 12:51
@baywet baywet requested a review from andrueastman February 3, 2022 20:53
@baywet baywet force-pushed the feature/error-description branch from 3bbb58d to b19d1ae Compare February 4, 2022 20:49
@baywet

This comment was marked as outdated.

@baywet baywet force-pushed the feature/error-description branch from b19d1ae to a155ac4 Compare February 7, 2022 18:24
@sebastienlevert
Copy link
Contributor

sebastienlevert commented Feb 7, 2022

What is the plan for the Error vs Error_escaped? MicrosoftGraphError ? Or something else? How it is right now feels awkward, but I love the approach to have native errors like that! If we can figure out the right type to use, I'm happy with the result!

Should I expect that the result would be :

try {
      const messagesResult = await client.usersById("vincent@biret365.onmicrosoft.com").mailFoldersById("Inbox").get({
        select: ["displayName2", "id"]
      });
      console.log(`messages Folder display name ${messagesResult?.displayName}`);
    } catch(err) {
        console.log(`Error: ${(err as Error).message}`);
    }

err.error being kinda weird, knowing we are expecting an error here.

@baywet

This comment was marked as outdated.

@baywet baywet marked this pull request as ready for review February 7, 2022 19:29
@baywet baywet requested review from andrueastman and jobala February 7, 2022 19:30
Signed-off-by: Vincent Biret <vibiret@microsoft.com>
Signed-off-by: Vincent Biret <vibiret@microsoft.com>
…ve response handler

Signed-off-by: Vincent Biret <vibiret@microsoft.com>
@baywet baywet force-pushed the feature/error-description branch from bf84350 to 3b5f7a3 Compare February 8, 2022 15:23
Signed-off-by: Vincent Biret <vibiret@microsoft.com>
@baywet baywet requested a review from nikithauc February 9, 2022 13:23
andrueastman
andrueastman previously approved these changes Feb 10, 2022
Copy link
Member

@andrueastman andrueastman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍🏼 Looks good to me.

docs/extending/kiotaabstractions.md Show resolved Hide resolved
jobala
jobala previously approved these changes Feb 10, 2022
@nikithauc
Copy link
Contributor

https://github.com/Microsoft/TypeScript/wiki/Breaking-Changes#extending-built-ins-like-error-array-and-map-may-no-longer-work

Object.setPrototypeOf(this, ApiError.prototype); 

This should be set in the Error classes extended from APIError. That is, in the generation process.

@nikithauc
Copy link
Contributor

@sebastienlevert
This is what you should expect once we fix the last conversion issues. This is because of OData conventions, object responses (the error in that case) are always wrapped in a parent object.

console.log(`Error: ${(err as Error).error?.message}`);

Error is the predefined JS error right? Does that accept an internal object, as far as I know it just accepts strings in the constructor?

@baywet I am missing this conversation in the chain. Are we continuing with Error_Escaped as seen in here in the samples?

Signed-off-by: Vincent Biret <vibiret@microsoft.com>
@baywet baywet dismissed stale reviews from jobala and andrueastman via 9713b9c February 10, 2022 18:35
Co-authored-by: Nikitha Chettiar <nikithauc@gmail.com>
@baywet
Copy link
Member Author

baywet commented Feb 10, 2022

@nikithauc, sorry I forgot this aspect, added

As per the error type it'll be

console.log(`Error: ${(err as ODataError).error?.message}`);

thanks to there latest changes. microsoft/OpenAPI.NET.OData#174

@sonarqubecloud
Copy link

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

89.6% 89.6% Coverage
0.0% 0.0% Duplication

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Csharp Pull requests that update .net code generator Issues or improvements relater to generation capabilities. Go Java TypeScript Pull requests that update Javascript code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add support for global error handling
5 participants