Skip to content

Conversation

@zedy-wj
Copy link
Contributor

@zedy-wj zedy-wj commented Jun 10, 2021

1.Upgrade EventGrid sdk from t1 to t2

2.Update files:

  • i. Fabrikam.Choreography.ChoreographyService/Controllers/ChoreographyController.cs
  • ii. Fabrikam.Choreography.ChoreographyService/Fabrikam.Choreography.ChoreographyService.csproj
  • iii. Fabrikam.Choreography.ChoreographyService/Services/EventRepository.cs
  • iv. Fabrikam.Choreography.ChoreographyService/Services/IEventRepository.cs
  • v. Frabrikam.Choreography.ChoreographyService.Tests/ChoreographyControllerFixture.cs
  • vi. Frabrikam.Choreography.ChoreographyService.Tests/Fabrikam.Choreography.ChoreographyService.Tests.csproj

@jongio for notification.

@zedy-wj zedy-wj marked this pull request as ready for review June 10, 2021 05:47
@jongio
Copy link

jongio commented Jun 14, 2021

@jsquire - can you please review?

@zedy-wj
Copy link
Contributor Author

zedy-wj commented Jun 18, 2021

Hi,@jsquire - Thanks for your review! Updated according to your comments.

using Microsoft.AspNetCore.Mvc;
using Microsoft.Azure.EventGrid;
using Microsoft.Azure.EventGrid.Models;
using Azure.Messaging.EventGrid;
Copy link

Choose a reason for hiding this comment

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

Truthfully, though we didn't change the behavior with the upgrade, we should really look at this controller in more depth. The logic is convoluted and it's demonstrating a really bad pattern of HTTP responses where any downstream failures are returned to the caller as an HTTP 400 (Bad Request) indicating "you did something wrong when sending your payload and you need to fix it for this to work."

Copy link

Choose a reason for hiding this comment

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

The coding conventions also often cut against best practices for C#. For example:

try
{
    ...
}
catch (Exception ex) when (ex is SomeExceptionType)
{
    ...
}

Should just be:

try
{
    ...
}
catch (SomeExceptionType ex)
{
    ...
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We have known the existence of this problem, we will improve the logic after completing all migrations. Our primary task is upgrading sdk from t1 to t2.

Copy link

Choose a reason for hiding this comment

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

I realize this request expands the scope, but let's get a better design here as well. Do you want to take a stab at it with Jesse's recommendation above or do you need further guidance?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We will take a stab at it, but it maybe takes a few time.

Copy link

Choose a reason for hiding this comment

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

Did you resolve this?

Copy link
Contributor

@v-yilinhu v-yilinhu Aug 10, 2021

Choose a reason for hiding this comment

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

@jongio,@jsquire We tried to make some changes in exception handing. Do you think this is ok or we need to subdivide BadRequest exception further?

@hallihan hallihan merged commit 2c006aa into mspnp:master Aug 17, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants