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

Controversial: Allow parameter values to be obtained along with query string #19334

Closed
wants to merge 1 commit into from

Conversation

ajcvickers
Copy link
Member

Issue #6482

var parameters = new Dictionary<string, object>();
var queryString = queryable.ToQueryString(parameters);

Not 100% convinced we should do this. Here's my thinking:

  • This can be useful for diagnostics/testing by providing the parameter values in a machine-readable, non string converted form. For example, imagine a profiling tool that can read the dictionary and make the parameter values available to run perf testing, get query plan, etc.
  • On the other hand, this could be used by people who want to use EF only to generate queries which are then executing somewhere else. This is not a goal of EF and the queries may not run in some other context, so it could give a false impression of what EF is designed to do.
  • But if we don't do this publicly, then I think the discussion on Provide a simple way to get SQL statements generated from EF queries #6482 indicates that people are going to continue to hack EF into doing it. This could end up being more painful than just supporting the feature.

Issue #6482

```C#
var parameters = new Dictionary<string, object>();
var queryString = queryable.ToQueryString(parameters);
```

Not 100% convinced we should do this. Here's my thinking:
* This can be useful for diagnostics/testing by providing the parameter values in a machine-readable, non string converted form. For example, imagine a profiling tool that can read the dictionary and make the parameter values available to run perf testing, get query plan, etc.
* On the other hand, this could be used by people who want to use EF only to generate queries which are then executing somewhere else. This is not a goal of EF and the queries may not run in some other context, so it could give a false impression of what EF is designed to do.
* But if we don't do this publicly, then I think the discussion on #6482 indicates that people are going to continue to hack EF into doing it. This could end up being more painful than just supporting the feature.
Copy link
Member

@roji roji left a comment

Choose a reason for hiding this comment

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

I don't think this adds anything more controversial beyond basic ToQueryString - if we allow getting SQL out. why not also parameters... So seems OK to me.

A thought... If we do this, should we stop adding parameters into the returned SQL as comments? I'm assuming that if people simply want to log SQL (and parameters), that's what we have logging for (including the new simple logging). So this would be more for programmatic access (profilers etc.), in which case the extra comments don't necessarily make sense?

@ajcvickers
Copy link
Member Author

@roji It's more controversial because it makes it even easier for people to abuse the API and use it for SQL execution in an application.

@ajcvickers
Copy link
Member Author

ajcvickers commented Dec 18, 2019

Notes from design meeting:

  • We will keep this API public and call it ToQueryString
    • We also discussed calling it ToDebugString, but that's less specific and discoverable. Also, other ToDebugString methods are internal.
  • We will not make this relational-specific
    • Non-relational providers may also have reasonable strings for their queries
  • We will remove the parameters--i.e. this PR
  • We will change the format of comments based on the suggestion here: DebugView for query and simple way to get SQL from an EF LINQ query #19324 (comment)
    • However, this is SQL Server specific, so the format only changes for SQL Server
    • We will still use comments since this isn't actually SQL that EF would normally generate. Changed my mind on this part.

ajcvickers added a commit that referenced this pull request Dec 19, 2019
Design meeting feedback on #19334 for #6482

* Formatting of the relational query string can now be overridden by providers
* SQL Server overrides this to create runnable `Parameter` declarations
* Tested by:
  * Executing the DbCommand we create when using `CreateDbCommand`
  * Executing a DbCommand created externally and given the generated query string
@ajcvickers
Copy link
Member Author

Superseded by #19368

@ajcvickers ajcvickers closed this Dec 19, 2019
ajcvickers added a commit that referenced this pull request Dec 23, 2019
Design meeting feedback on #19334 for #6482

* Formatting of the relational query string can now be overridden by providers
* SQL Server overrides this to create runnable `Parameter` declarations
* Tested by:
  * Executing the DbCommand we create when using `CreateDbCommand`
  * Executing a DbCommand created externally and given the generated query string
@smitpatel smitpatel deleted the DragonAttack1216 branch January 2, 2020 19:38
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.

3 participants