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

Added "Select" and "AndSelect" to "PetaPoco" #2770

Merged
merged 3 commits into from Jul 15, 2018

Conversation

ssougnez
Copy link

@ssougnez ssougnez commented Jul 13, 2018

Prerequisites

Description

Currently, "PetaPoco" can be used to execute SQL queries by using predicates. For example:

new Sql().Select('*').From<ContentDto>()

However, when only some columns must be retrieved, they have to be specified as string parameters, which is error prone.

new Sql().Select('[cmsContent].[nodeId]').From<ContentDto>()

If a mistake is made in "[cmsContent].[nodeId]", the compiler won't complain but the query won't succeed. It implies that if a database column changes, it's pretty hard to find and replace all occurence of that column name.

This PR adds the possibility to select columns using one or several predicates:

new Sql().Select<ContentDto>(c => c.NodeId, c => c.ContentTypeId)

If no delegate is passed to this function, it selects all the fields ("[table].*).
'AndSelect" must be used to select columns from more than one table (in the case of a join for example):

new Sql().Select<DocumentDto>(d => d.NodeId, d => d.Published)
                .AndSelect<ContentVersionDto>(cv => cv.Id)
                .From<DocumentDto>()
                .InnerJoin<ContentVersionDto>()
                .On<DocumentDto, ContentVersionDto>(left => left.VersionId, right => right.VersionId);

These two methods have been implemented in two versions:

  • One that does not expect a ISqlSyntaxProvider argument that is marked as obsolete (just to keep some kind of continuity for people not using ISqlSyntaxProvider... and allowed me to use the current test method as they don't use ISqlSyntaxProvider right now)
  • One that does expect a ISqlSyntaxProvider argument that is not marked as obsolete.

The following test have also been added to ensure the functionality behaves as expected:

  • Can_Use_Select_With_Star_And_Predicate
  • Can_Use_Select_With_One_Column_And_Predicate
  • Can_Use_Select_With_Multiple_Column_And_Predicate
  • Can_InnerJoin_With_Select_And_AndSelect

Tested on the v7.12, these tests (and all the others from the same class) execute correctly.

sebastien-sougnez and others added 2 commits July 13, 2018 15:10
Copy link
Member

@nul800sebastiaan nul800sebastiaan left a comment

Choose a reason for hiding this comment

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

Thanks @ssougnez - build is running just fine now and tests are passing 👍

I think this looks good, but I left 2 comments for you to fix some things up!

/// <param name="sql">Sql object</param>
/// <param name="fields">Columns to select</param>
/// <returns></returns>
[Obsolete("Use the overload specifying ISqlSyntaxProvider instead")]
Copy link
Member

Choose a reason for hiding this comment

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

I see why you did this, but instead, let's just fix the source of the problem. I'd add a field on the BaseUsingSqlCeSyntax class that just initiliazes a new SqlServerSyntaxProvider() which you can then use in your tests. It's not very nice to introduce a new public method that is immediately obsolete. ;-)

Copy link
Author

Choose a reason for hiding this comment

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

Ok, I'll have a look at that :-D

Copy link
Member

Choose a reason for hiding this comment

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

Should be SqlCeSyntaxProvider of course since the tests are using CE. :-)

@@ -0,0 +1,8 @@
<?xml version="1.0"?>
Copy link
Member

Choose a reason for hiding this comment

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

No need to commit this file.

Copy link
Author

Choose a reason for hiding this comment

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

Well, actually, I have no idea what that file is :-D

What am I supposed to do with it? Just delete it?

Copy link
Member

Choose a reason for hiding this comment

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

Haha, then don't commit it! 😉

I think it's Visual Studio "trying" to be helpful. :) Yep, just delete it!

Copy link
Author

Choose a reason for hiding this comment

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

Sorry, Umbraco is the first project I'm contributing to via Github. It's not that easy to understand quickly :-D But I'd like to contribute more to Umbraco, so I'll try to pay attention to these kind of things.

Copy link
Member

Choose a reason for hiding this comment

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

Not to worry, we're here to help and so far you've done great! 👍

@nul800sebastiaan nul800sebastiaan merged commit 66b2ad9 into umbraco:dev-v7 Jul 15, 2018
@nul800sebastiaan
Copy link
Member

Thanks @ssougnez - all good now and merged, top job! 🏅

@ssougnez ssougnez deleted the temp-U4-11513 branch July 17, 2018 13:40
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