Add support for case insensitive dynamic pocos

[Curlack]

Split Oracle scripts and tests to test usage of "ordinary" and "delimited" identifiers.

Add support for case insensitive dynamic objects. ExpandoPoco vs ExpandoObject. To be used by OracleOrdinaryDatabaseProvider.

Add UseOrdinaryIdentifiers flag to IProvider to provide a global means of controlling the activation of case insensitive dynamic objects.

Add ignoreCase optional parameter to all Database constructors (except the ones taking in a provider) and Poco factory methods.

Add UsingCaseInsensitiveProvider configuration extension.

Switch between OracleDatabaseProvider and OracleOrdinaryDatabaseProvider based on UseOrdinaryIdentifiers flag when Resolving Provider.

[Ste1io]

@Curlack, looking forward to taking a look at the changes. There is a slight issue with commits from the previous PR being included in the new one, likely due to the hard reset earlier (easily fixed, though). Looking at the commit hashes in this case, it appears that the new commits were done without first rebasing onto upstream's feature/oracle-support branch following the final force-push you did from the Github web UI (notice commit e57bf10 in this PR, which was actually replaced by b36c22b when you did the final force-push in #707).

Should be fairly painless to fix, you probably can just c/p the git commands below all at once and call it a day. I added inline comments so you know what's going on, but essentially we're just making a new branch without your new changes, applying only your new commits that we want to it, then replacing your original feature branch with the new one.

# Create a new private feature branch ("fix-oracle-quoting") from upstream's "feature/oracle-support" branch; 
# this gives you a clean slate based on the upstream branch without any of your new commits
git checkout -b fix-oracle-quoting upstream/feature/oracle-support
# Now we apply your changes for this PR (in order) to this new private feature branch; 
# this is effectively the same as what rebasing does, by replaying the commits from another branch 
# as if they all occurred in order on a single branch.
git cherry-pick 82f67b8 5cf2dea df1d8b2 0addb74
# In this case, we need to fix up your "feature/oracle-support" private branch which you opened the PR from, 
# so just overwrite it with our new feature branch "fix-oracle-quoting"
git branch -f feature/oracle-support fix-oracle-quoting
# Force push to your remote and the PR should be fixed
git push origin feature/oracle-support --force

For what it's worth, git can be a bit of a brainf**k between local, remote, origin, upstream, and who is mine and theirs LOL, so don't feel bad.

I generally I create a short-lived private feature branch for each PR; it's not necessary, but it does encapsulate that PR's changes separately from the target branch fairly well.

• Once the PR is approved, I can delete that private feature branch, sync my fork with upstream (same as merging upstream's changes into my branch), and create a new private feature branch for the next PR.
• While working on my short-lived feature branch, if there are changes upstream to the branch I intend to open my PR against, I just rebase onto upstream's branch, that way those commits get applied to my feature branch alongside my own.

[Curlack]

Thanks for all the help and patience. I'm busy trying to fix as you indicated, but it seems git doesn't know about branch upstream/feature/oracle-support. Is there something else I'm missing?

[asherber]

I've been following this from the sidelines, but looking now at the code changes, I have reservations about what's being added -- seems like a lot of things that shouldn't really be the business of this library.

Many databases have case-sensitivity issues, and I think it's up to the user to set up their default mapper to handle it. I don't like adding an ignoreCase parameter in the Database constructors (and the new extension method) when that parameter only has an effect for Oracle.

[asherber]

Also, adding a property to IProvider is a breaking change.

[Ste1io]

Thanks for all the help and patience. I'm busy trying to fix as you indicated, but it seems git doesn't know about branch upstream/feature/oracle-support. Is there something else I'm missing?

I'm on mobile so double check for typos... Make sure you fetch upstream first. Also this repo needs set as upstream remote if it isn't already; check with git remote -v. If none set, git remote add upstream https://github.com/CollaboratingPlatypus/PetaPoco.git.

Run git fetch upstream, then you should be good.

[Curlack]

I've been following this from the sidelines, but looking now at the code changes, I have reservations about what's being added -- seems like a lot of things that shouldn't really be the business of this library.

Many databases have case-sensitivity issues, and I think it's up to the user to set up their default mapper to handle it. I don't like adding an ignoreCase parameter in the Database constructors (and the new extension method) when that parameter only has an effect for Oracle.

Also, adding a property to IProvider is a breaking change.

Noted. I was expecting this, hence me mentioning the eyebrows earlier.
Just wanted to somehow demonstrate one way of solving the dynamic case sensitivity issue and also how it ties into the database, provider and factories. Out of all the things I tried, this seemed the cleanest, believe it or not.

Open to suggestions at this point, but completely agree with you. Of the two extremes, I first went this route then we'll work our way back to the ideal.

[Curlack]

Thanks for all the help and patience. I'm busy trying to fix as you indicated, but it seems git doesn't know about branch upstream/feature/oracle-support. Is there something else I'm missing?

I'm on mobile so double check for typos... Make sure you fetch upstream first. Also this repo needs set as upstream remote if it isn't already; check with git remote -v. If none set, git remote add upstream https://github.com/CollaboratingPlatypus/PetaPoco.git.

Run git fetch upstream, then you should be good.

I think I did it! ...,but you'll be the judge when you get a chance.

PS: When you do the review, don't review with the aim to approve the PR this time, but rather as a way to show you my changes to better collaborate. Not sure if there is another way to share code that does involve a PR.

[Ste1io]

Welp, replying on my phone, clearly haven't learned my lesson... Disregard the accidental PR status change. I'll be at pc in a couple hrs, going to refrain from further participation until then for the sanity of all of you lol.

[asherber]

@Curlack I may be missing something, but the fact that dynamic objects are not inherently case-insensitive isn't an issue for this library. PetaPoco maps database values to POCOs, and I think that end users could map things to a case-insensitive dynamic with current functionality, if they wanted. At most, it would take a few lines in an extension method. Providing a case-insensitive dynamic POCO seems outside the scope of this library.

I've never used Oracle, so there are probably some things specific to that db that I'm unaware of. Reading your comment about delimited vs. ordinary, it sounds similar to how Postgres handles things, which I am familiar with. IMO, it's the job of the user to understand that PP always escapes identifiers and plan accordingly with respect to database design and PetaPoco mapper. For example, since Postgres folds unquoted names to lowercase, it's common to name tables and columns all in lowercase. But since I know that PetaPoco will quote all identifiers, and since POCO class and property names are PascalCase, I always attach a mapper to my Database object that similarly lowercases table and column names, since they will be quoted and hence case-sensitive. Alternatively, I could have used a custom provider that makes EscapeSqlIdentifier a no-op, which I think you discuss elsewhere.

I'll also add that an option on the provider to "ignore case" seems vague. For example, queries against a text or varchar column in Postgres use case-sensitive matching. If I tell PetaPoco to ignore case, will it make these queries case-insensitive as well?

(This is also something that can be solved with current functionality. I defined my columns as citext, which is a case-insensitive text type in Postgres, but since the incoming parameter also has to be citext in order for this to work, I added a handler to OnCommandExecuting in my Database object that loops through query params and changes the type of all string params to citext.)

So...I don't see anything here that adds relevant functionality to the library, and I would currently vote against merging this PR. If you can explain succinctly what I'm not seeing, I'm open to having my mind changed.

[Curlack]

Providing a case-insensitive dynamic POCO seems outside the scope of this library.

Agreed.

I'll also add that an option on the provider to "ignore case" seems vague

Couldn't think of a better name, but mentioned in the XML comment that it's whether to use case insensitive dynamic object. This only applies to the property accessors in the dynamic object coming from the PocoData factory for typeof(object) e.g. Oracle still gives {"ID": 1}, but we can get/set poco.Id.

I don't see anything here that adds relevant functionality to the library, and I would currently vote against merging this PR.

Your comments have been very helpful and actually gave me a better idea. So agreed, no merge.

Summary

From Oracle's perspective, delimited identifiers are the exception to the rule. My approach was based on this, which led me down the wrong path. From PP's perspective, ordinary identifiers are actually the exception to the rule. For my next trick, I'll be taking this approach. I'm gonna have a look at some of the Postgres tests and core implementation and start reworking these changes. @Ste1io don't even bother reviewing this PR.

The changes are too invasive with no real value to the core concept of this library. They also contain breaking changes with no real justification

New game plan:

• I will start "vanilla" i.e. manually remove violating changes based on git diffs. I will keep the new Oracle integration test split and just move the 'OracleOrdinaryDatabaseProvider' into the integration tests project, as a custom provider (for the "Ordinary" slice of the Oracle tests).
• See how many "Delimted" tests fail and tweak where necessary to make them pass. These tests should at least be supported by the library out of the box (except for the ones using dynamic).
• Take another crack at the "Ordinary" slice and see where we're at as far as failing tests are concerned.
• Provide missing functionality to custom provider and custom mapper, to get as many tests to pass as is possible.
• Modify remaining failing tests to demonstrate deviations/workarounds required to arrive at the destination.
• Generate all scenarios around casing for Oracle vs C# and post them in the original issue.

Thanks again for all the help, really appreciate it.

[asherber]

@Curlack Thanks for your comments. I haven't looked in detail at the Oracle code, but from a high level, I would suggest focusing on getting current tests to pass out of the box. Maybe there's something that needs to be restructured in the tests in order to accommodate Oracle, though I'm pretty sure there was a point at which they all passed. Maybe take a look at NPoco to see how they do things. (NPoco is a well-maintained fork of the project this repo also came from.)

If you think there's a need to provide alternative approaches for Oracle, consider whether the better place for that might be a separate library or even just a page in the Wiki with tips and hints. Consider also whether Oracle is more in need of this attention than other databases; I don't see any open issues in the repo relating to these things.

[kchanghui]

PetaPoco 对postgres 数据库支持有bug , 当插入数据表, 数据表会带有引号“myTable”, 报错信息： Npgsql.PostgresException:“42P01: 关系 "myTable" 不存在”； 实际上表是存在的，查询没问题， 当执行insert，update ， delete ,就会包这样的错误