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

Add support for parameterized queries #61

Merged

Conversation

joakimhew
Copy link
Contributor

@joakimhew joakimhew commented Oct 1, 2017

With support for parameterized queries, InfluxDB can be queried in the following manner:

var serialNumber = "F2EA2B0CDFF"
var query = "SELECT * FROM cpuTemp WHERE \"serialNumber\" = @SerialNumber";
var response = await influxDbClient.Client.QueryAsync(
     query: query, 
     param: new 
     { 
           @SerialNumber = serialNumber 
     }, 
     dbName: "yourDbName");

The signature for the new overload is:

public Task<IEnumerable<Serie>> QueryAsync(string query, object param = null, string dbName = null, string epochFormat = null, long? chunkSize = default(long?));

The supported types of parameters are primitive types as well as Strings. Trying to use un-supported types will throw a NotSupportedException.

All strings are sanitized.

Joakim Hansson added 2 commits October 1, 2017 02:43
- Added string extension to sanitize queries.
@joakimhew joakimhew changed the title Adds support for parameterized queries Add support for parameterized queries Oct 1, 2017
@tihomir-kit tihomir-kit merged commit 6728262 into tihomir-kit:develop Oct 2, 2017
@tihomir-kit
Copy link
Owner

Well man, this is some seriously nice stuff. Thanks! I'll look at it more closely tomorrow, but I like what I've been able to see by just quickly checking it out. Cheers

@joakimhew
Copy link
Contributor Author

Just glad I could help man :) I've tested it quite a lot today and it seems to be working good. One thing worth iterating over though, is the behaviour of non-used properties. As an example. Let's say you're passing this model to the param:

var filter = new Filter 
{
     SerialNumber = "abcd1234",
     SensorId = "4321dcba"
} ;

but the query only uses an identifier for SerialNumber like so:

var query = "SELECT * FROM Measurement WHERE serialNumber = @SerialNumber";

Maybe it should see this as valid and just ignore the SensorId? I need to use this for work tomorrow so I'll keep you updated 👍

Cheers.

@joakimhew joakimhew deleted the feature/parameterized_queries branch October 4, 2017 12:36
@joakimhew joakimhew restored the feature/parameterized_queries branch October 4, 2017 12:36
@tihomir-kit
Copy link
Owner

My 2c - I think we should not throw exceptions if a user passes an object that has extra properties which are not part of the query. To me this is completely fine and should not be sanitized.

@tihomir-kit
Copy link
Owner

tihomir-kit commented Oct 4, 2017

Ok, I've looked at what you've done and I think it's great. :)

It's been on my mind to implement exactly this stuff actually for some time now, thanks.

I applied some minor renamings in the code so it matches the rest of the style of the codebase. A small breaking change is renaming of .RecordsAs<T>() to simply .As<T>(). I think it's simpler, not redundant, and still very clear. I also renamed the BuildParametarizedQuery to simply BuildQuery but I converted it to an extension method so the usage syntax is now something like queryTemplate.BuildQuery(parameters) which I believe is very clear. What do you think?

Btw one of your tests is failing - the ArgumentException for the QueryHelper. Would you please be able to take a look perhaps when you have the time (just pull from the latest dev branch)? I don't want to mess with the logic you had in mind for this.

@joakimhew
Copy link
Contributor Author

@pootzko Awesome! I've looked through your changes and now I have a better feel for the codestyle in the project. Thanks for that! :)
All looks good to me man!

The test failing is most likely due to the correction in some regex that I did (0d12aba), sorry for that. I'll fix it later!

@joakimhew
Copy link
Contributor Author

I'm terribly sorry that I haven't had the time to go through it man. Did you manage to find time? Otherwise I'll see if I can get some time tonigh. I've got a nice little bug fix that I think you'll enjoy haha.. I just added fill(previous) to my query which will return null values from influx. SerieExtensions doesn't like this when it tries to use Convert.ChangeType https://github.com/joakimhew/InfluxData.Net/blob/77c406be6cc0145d4acda744587febea5350325a/InfluxData.Net.InfluxDb/Helpers/SerieExtensions.cs#L65.

A solution for this is to check if the value is of type value and if so, make sure to use the Activator class to create an instance for the value:

if(propType.IsValueType && value[columnIndex] == null)
{
     value[columnIndex] = Activator.CreateInstance(propType);
}

I'll get a PR going as soon as I can with the fix and add the related tests 👍

@tihomir-kit
Copy link
Owner

Don't worry.

I just commented out that specific test that's failing, so when you have the time, I'll gladly accept another PR. :) It's easy to redeploy a simple fix. Thnx!

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

2 participants