-
Notifications
You must be signed in to change notification settings - Fork 874
Some fixes #10
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
Some fixes #10
Conversation
|
Hi, Andrey! Thank you very much for your patches. I'll apply them to our cvs repository and then I'll merge it to git. Can you provide a more detailed explanation of the fix number 1 so I can add in the changelog and in the release notes? About the fix number 3, I think we should provide a connection string switch to enable the developer to use the current behavior. This way I think people who can't change their programs to adapt to new quoting rules still can use newer Npgsql versions. |
|
I just sent a mail to Josh Cooley. He is the developer who coded Entity Framework support so he can also have a look and review it. |
|
Hi. It's a bit hard to provide extended explain for 1. I think it's the bug. I've try to find UnitTests for EntityFramework support, but I can't understand how to run it. Can you help me? Then I can totally check my fix in correct way. Backslash quoting problem is more complex. My fix breaks executing of test Bug1010557BackslashGetDoubled and InsertStringWithBackslashes in CommandsTests. |
|
I want to explore slash problem more closer. I need to know how PostgreSQL return slashes from server to client: there can I find this in Npgsql code. |
|
I can help you run the test suite we have for Npgsql. What steps have you already done to set up Npgsql nunit tests? Because of this backward compatibility problems I suggest we create a connection string parameter to control this backslash behavior. This way, clients expecting previous behavior would only need to change the connection string. |
|
Npgsql receive data from the server here: https://github.com/franciscojunior/Npgsql2/blob/master/src/Npgsql/NpgsqlState.cs#L828 Another tool you can use to inspect postgresql data transmission is WireShark, (previous know as Ethereal). It already recognizes postgresql protocol and it very useful. I hope it helps. |
|
I run all test from NpgsqlTests2010 projects, but this solutions doesn't contain any EntityFramework test. Also this solution contains xmlModel folder wich might be a base for EF tests, but I don't understand how it's been used. I suggest that Npgsql already contains such connetion string parameter NpgsqlConnector.CheckStringConformanceRequirements() but it's uncomplete proccessed at every part of the code. I'll try to discover entire logic and create correct fix. Thanks for advice about code and WireShark. |
|
Hmmm, these files were created by Josh Cooley but I think they aren't automated tests. I'll check with him how to run those tests. |
|
Hi, Andrey. About your backslash patch, I also received another report of problems with backslashes when running Npgsql with entity framework. http://pgfoundry.org/forum/forum.php?thread_id=15533&forum_id=518&group_id=1000140 I think we should check how to get the preceding 'E' in front of queries created by Npgsql entity framework support. This way we wouldn't need to change current code which already provides support for current and previous postgresql. What do you think? |
|
Hi. |
|
Hi. |
|
That's great, Andrey! I'll commit it to cvs and let you know. |
|
Agree |
|
Great! Will commit this revised version to cvs as soon as I get back home. Here in my job I can't access cvs. :( |
|
Hi, while preparing to commit, I notice a little problem: Npgsql supports postgresql since 7.x and this version doesn't support the E'string' syntax. We would need to check how to send the information if the E'string' syntax is supported or not so that the |
|
I think we would need to make a better handling of those strings. I think we need to change the ConvertToBackend method which would be used by both EF queries and NpgsqlCommand queries. |
|
I'm still trying to figure out a way to handle that. When doing the output from EF in the ConstantExpression class, there is no connection/context information available. Or at least, I couldn't find it. I sent a mail to Josh Cooley who coded this part asking him if he has any idea about how to handle that. |
|
Yesterday I was playing with this issue and I think we could use some methods of NpgsqlServices class to get the information about the ConformantStrings so the writeSql can create the string literal with the proper escape syntax. Inside NpgsqlServices there are the methods called GetDbProviderManifestToken and GetDbProviderManifest. I think we could use them to retrieve the status of the ConformantStrings and pass down the chain of sql generators in the TranslateCommandTree method. |
|
After getting feedback from Josh, I think we can try the following idea: The current code already gets the server version. With this information, we can do the following: For server versions 7.x we write the string without the escape syntax. With versions greater than 8.0, we write the string with the escape syntax (E'string value'). What do you think? |
|
After asking more questions to Josh. he told me there are a lot of metadata information used by EF that he isn't sure if they are available to Npgsql in the Postgresql 7.x. We would need to do more tests. For while, I'm applying your patch which adds the escape syntax to EF code as, currently, EF support is only available in postgresql 8.x. As escape syntax is also supported on postgresql versions 8.x and above, we can say that this problem right now is fixed. If we find a way to support EF with 7.x, we will add a condition to this escape syntax. |
|
If it's fixed it might be good to close this PR, so it doesn't look like the project won't merge PRs ;) |
|
Agreed! I'll check the current state of those patches and write them here and will close this PR if everything is already applied. Thanks for the heads up, @FransBouma ! |
8bcb1ca to
1bed81a
Compare
1236d1f to
e8ae2e0
Compare
2fc8944 to
3eb6fa4
Compare
c3f6238 to
e680736
Compare
|
@Emill, @franciscojunior, can any of you take a look if this PR contains salvageable stuff for 3.0? Otherwise I'll close it... |
Hi!
I've make some improoves to Npgsql.
First and second commits are universal and works good.
Third may break backward compatibility. But it's correct for actual versions of PostgreSQL. If you make me advice how to do this more correct I can rewrite my code.