Skip to content

Added an option to suppress exception details#3011

Merged
YohDeadfall merged 4 commits into
npgsql:devfrom
dwat001:Issue2501_ExceptionDetailHasProductionData_Master
Jun 15, 2020
Merged

Added an option to suppress exception details#3011
YohDeadfall merged 4 commits into
npgsql:devfrom
dwat001:Issue2501_ExceptionDetailHasProductionData_Master

Conversation

@dwat001

@dwat001 dwat001 commented Jun 12, 2020

Copy link
Copy Markdown
Contributor

As per comments in the Issue, this PR deals with both PostgresException.Details and the logging of command parameters.

This is related to PR #3008 which does the same for the 3.X branch.

@dwat001 dwat001 requested review from YohDeadfall and roji as code owners June 12, 2020 01:15
@dwat001 dwat001 mentioned this pull request Jun 12, 2020
Closed
@YohDeadfall YohDeadfall changed the title Issue2501 exception detail has production data master Added an option to suppress exception details Jun 12, 2020
@YohDeadfall YohDeadfall changed the base branch from master to dev June 13, 2020 12:50
@YohDeadfall

YohDeadfall commented Jun 13, 2020

Copy link
Copy Markdown
Contributor

@dwat001 I forgot that we use dev instead of master as an active branch. I changed the target, but could you rebase your changes? Sorry for this.

@dwat001
Conditionaly suppress exception detail (npgsql#2501)
a1afd65 
Based on a new ConnectionString property SuppressDetailInPostgressError drop the DETAILS field sent by postgres as this often contains user data which may be sensitive.

# Conflicts:
#	src/Npgsql/PostgresException.cs
@dwat001 dwat001 force-pushed the Issue2501_ExceptionDetailHasProductionData_Master branch from da1ea0e to a1afd65 Compare June 14, 2020 23:05
@dwat001
Conditionaly suppress logging of parameters (npgsql#2501)
6ce1f41 
Based on a new connection string property, suppress the logging of Command parameters.
@dwat001

dwat001 commented Jun 14, 2020

Copy link
Copy Markdown
Contributor Author

@dwat001 I forgot that we use dev instead of master as an active branch. I changed the target, but could you rebase your changes? Sorry for this.

@YohDeadfall I have rebased onto dev, which branch should I be targeting to get this onto the 4.x line as well as dev?
(Sorry for the confusion around 3.x vs 4.x)

@YohDeadfall

YohDeadfall commented Jun 15, 2020

Copy link
Copy Markdown
Contributor

Thanks!

After merging this, the changes will appear in 5.0. To have them in 4.x versions we will backport/cherry-pick commits on our side, so no additional actions required for you.

@dwat001

dwat001 commented Jun 15, 2020

Copy link
Copy Markdown
Contributor Author

Thanks @YohDeadfall, could you please ping #2501 or this PR once it is cherry-picked?
If there is back-porting to do I would be happy to do that.

For clarity is there any further work required on this PR for it to be ready for merging?

Thank you.

@YohDeadfall

YohDeadfall commented Jun 15, 2020

Copy link
Copy Markdown
Contributor

On backporting we add a comment to specify a cherry pick commit and the corresponding branch.

YohDeadfall
YohDeadfall reviewed Jun 15, 2020
View reviewed changes

@YohDeadfall YohDeadfall left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Looks good, but let's add a test to prevent a future regression. It should be in ExceptionTests.

Comment thread src/Npgsql/BackendMessages/ErrorOrNoticeMessage.cs Outdated
break;
case ErrorFieldTypeCode.Detail:
detail = buf.ReadNullTerminatedStringRelaxed();
if(suppressDetailInPostgressError && string.IsNullOrEmpty(detail) == false)

@YohDeadfall YohDeadfall Jun 15, 2020

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

It would be better to use the negation operator here rather than comparison, but it's a bit.

dwat001 added 2 commits June 15, 2020 21:39
@dwat001
Test surpression of exception detail (npgsql#2501)
fd412b6
@dwat001
Code Tidy (npgsql#2501)
f154c73 
As per review use not operator instead of comparison to false.
@dwat001 dwat001 requested a review from YohDeadfall June 15, 2020 09:44
@YohDeadfall YohDeadfall linked an issue Jun 15, 2020 that may be closed by this pull request
PostgreSQL returns inserted data in exception if insert fails #2501
Closed
@YohDeadfall YohDeadfall merged commit adefbbf into npgsql:dev Jun 15, 2020
YohDeadfall pushed a commit that referenced this pull request Jun 15, 2020
@dwat001 @YohDeadfall
Added an option to suppress exception details (#3011)
6fe5cde 
Conditionally suppress exception details

(cherry picked from commit adefbbf)
@YohDeadfall

YohDeadfall commented Jun 15, 2020

Copy link
Copy Markdown
Contributor

Backported to 4.1.4 via 6fe5cde.

YohDeadfall pushed a commit that referenced this pull request Jun 15, 2020
@dwat001 @YohDeadfall
Added an option to suppress exception details (#3011)
3f81746 
Conditionally suppress exception details

(cherry picked from commit adefbbf)
@YohDeadfall

YohDeadfall commented Jun 15, 2020

Copy link
Copy Markdown
Contributor

To 4.0.11 it was backported via 3f81746.

@dwat001

dwat001 commented Jun 15, 2020

Copy link
Copy Markdown
Contributor Author

Thank you for your time on this @YohDeadfall .

@dwat001 dwat001 deleted the Issue2501_ExceptionDetailHasProductionData_Master branch June 15, 2020 21:14
roji
roji reviewed Jun 19, 2020
View reviewed changes
Comment thread src/Npgsql/NpgsqlCommand.cs
sb.Append("\t").AppendLine(s.SQL);
var p = s.InputParameters;
if (NpgsqlLogManager.IsParameterLoggingEnabled && p.Count > 0)
if (NpgsqlLogManager.IsParameterLoggingEnabled && p.Count > 0 && excludeParametersFromLogs == false)

@roji roji Jun 19, 2020

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This means that NpgsqlLogManager.IsParameterLoggingEnabled has to be enabled regardless of what's on the connection string - that seems to make the new connection string option pretty useless. I think it should be possible to simply enable parameter logging via one or the other mechanism.

@dwat001 dwat001 mentioned this pull request Jul 13, 2020
Open
@roji roji mentioned this pull request Sep 15, 2021
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@roji roji roji left review comments

+1 more reviewer

@YohDeadfall YohDeadfall YohDeadfall approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@dwat001 dwat001

@YohDeadfall YohDeadfall

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

PostgreSQL returns inserted data in exception if insert fails

3 participants

@dwat001 @YohDeadfall @roji