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

OTC-586 Implement "using" for all disposable elements (streams, connections, writers) #151

Merged
merged 4 commits into from May 6, 2022

Conversation

malinowskikam
Copy link
Contributor

@malinowskikam malinowskikam commented Apr 29, 2022

https://openimis.atlassian.net/browse/OTC-586

Changes:

  • Updated SQL helpers to handle connections with using
  • Updated XML serializations to handle writers and streams with using
  • Updated WebRequests to handle streams with using
  • Added LoggerFactory to various components
  • Added log calls to various error handlers
  • Refactored GepgFileLogger.cs
  • Updated missing awaits in async tasks
  • Updated missing override operators

@delcroip
Copy link
Member

There is lots of changes, I guess I need someone more experienced in c# to review and maybe to test.

I would like to have some test on the sit server.

@hirensoni913 can you see what you can do ?

@@ -25,25 +25,8 @@ public ImisCoverage(IConfiguration configuration)
SqlParameter[] sqlParameters = {
new SqlParameter("@InsureeNumber", insureeNumber),
};
DataTable response = new DataTable();

try
Copy link
Contributor

Choose a reason for hiding this comment

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

Why the Try/Catch block is removed? Can't we log the error rather than removing it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Because this block just rethrowed the sql exception and replaced any other exception with empty generic exception, crashing the endpoint and hiding the cause. Without it the escaped exception will be caught and logged

Copy link
Contributor

@hirensoni913 hirensoni913 left a comment

Choose a reason for hiding this comment

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

@malinowskikam can you add the proper exception handling in the catch blocks?

@hirensoni913
Copy link
Contributor

@malinowskikam Can you also look in to the "GepgFileLogger" file. This file contains lots of IO operations. We need to use the using block in it + log the exceptions in proper way

@malinowskikam
Copy link
Contributor Author

@hirensoni913 I updated the empty error handlers in the payment controller and logic. Also I refactored GepgFileLogger

@delcroip delcroip merged commit bd0c070 into develop May 6, 2022
@delcroip delcroip deleted the feature/OTC-586 branch May 6, 2022 13:19
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.

None yet

4 participants