Skip to content

Conversation

@Sielnix
Copy link

@Sielnix Sielnix commented Jan 9, 2024

Description

There's a bug, that having specified database or schema name in connection string, that contains any lowercase letter, throws exception during connection opening.

This PR fixes that.

Checklist

  • Code compiles correctly
  • Code is formatted according to Coding Conventions
  • Created tests which fail without the change (if possible)
  • All tests passing (dotnet test)
  • Extended the README / documentation, if necessary
  • Provide JIRA issue id (if possible) or GitHub issue id in PR name

@Sielnix Sielnix requested a review from a team as a code owner January 9, 2024 18:42
@github-actions
Copy link

github-actions bot commented Jan 9, 2024

CLA Assistant Lite bot All contributors have signed the CLA ✍️ ✅

@Sielnix
Copy link
Author

Sielnix commented Jan 9, 2024

I have read the CLA Document and I hereby sign the CLA


private string EscapeDbObjectForUrl(string name)
{
return name == string.Empty ? name : $"\"{name.Replace("\"", "\"\"")}\"";
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please make sure the name isn't already wrapped with quotes.
Make sure if connection tests are passing after this fix.

queryParams[RestParams.SF_QUERY_WAREHOUSE] = properties.TryGetValue(SFSessionProperty.WAREHOUSE, out warehouseValue) ? warehouseValue : "";
queryParams[RestParams.SF_QUERY_DB] = properties.TryGetValue(SFSessionProperty.DB, out dbValue) ? dbValue : "";
queryParams[RestParams.SF_QUERY_SCHEMA] = properties.TryGetValue(SFSessionProperty.SCHEMA, out schemaValue) ? schemaValue : "";
queryParams[RestParams.SF_QUERY_DB] = EscapeDbObjectForUrl(properties.TryGetValue(SFSessionProperty.DB, out dbValue) ? dbValue : "");
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can you provide some unit tests for this change?

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.

2 participants