-
Notifications
You must be signed in to change notification settings - Fork 52
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
Add support for parameterized queries #61
Conversation
- Added string extension to sanitize queries.
- Add integration tests
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 |
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:
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. |
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. |
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 Btw one of your tests is failing - the |
@pootzko Awesome! I've looked through your changes and now I have a better feel for the codestyle in the project. Thanks for that! :) 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! |
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 👍 |
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! |
With support for parameterized queries, InfluxDB can be queried in the following manner:
The signature for the new overload is:
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.