Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with
or
.
Download ZIP

Loading…

Validate UpdateDataExpressions: enforce that they either specify AllRows... #321

Merged
merged 1 commit into from

2 participants

@rytmis

Enforce that UpdateDataExpressions either specify AllRows or a nonempty Where (but not both). This came up when I was first using FM to update a bunch of rows -- I didn't specify either of the above, expecting it to work like SQL, and the end result was a nasty NRE.

Lauri Kotilainen Validate UpdateDataExpressions: enforce that they either specify AllR…
…ows or a nonempty Where (but not both)
a91e140
@daniellee
Collaborator

This pull request looks fine but I don't think it solves your problem. I'm still getting exceptions when trying to use UpdateDataExpressions without AllRows or a Where. (I tested it with the Console runner)

As far as I can see, the ICanBeValidated interface is only for testing purposes. I am going to check that by asking @schambers if he remembers what the original intent was for the ICanBeValidated interface. Otherwise, maybe we should run validation in the runners and catch some errors earlier. This could lead to nicer error messages when FM fails.

@rytmis

Well that's embarrassing. I'll have to give it an actual try then with an actual db. I figured since the error messages are appended in the validators, that would be the place to do this. :-)

@daniellee
Collaborator

I'm going to add a validation step to the MigrationRunner class that uses these validations. So I'm merging this in and the error message can be used instead of throwing a hard-to-understand exception with stack trace.

Thanks for your Pull Request!

@daniellee daniellee merged commit 0eca4f6 into schambers:master
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Oct 23, 2012
  1. Validate UpdateDataExpressions: enforce that they either specify AllR…

    Lauri Kotilainen authored
    …ows or a nonempty Where (but not both)
This page is out of date. Refresh to see the latest.
View
3  src/FluentMigrator.Tests/FluentMigrator.Tests.csproj
@@ -218,6 +218,7 @@
<Compile Include="Unit\Builders\Schema\SchemaExpressionRootTest.cs" />
<Compile Include="Unit\Definitions\IndexDefinitionTests.cs" />
<Compile Include="Unit\Expressions\DeleteDefaultConstraintExpressionTests.cs" />
+ <Compile Include="Unit\Expressions\UpdateDataExpressionTests.cs" />
<Compile Include="Unit\Generators\GenericGenerator\GenericGeneratorTests.cs" />
<Compile Include="Unit\Generators\Firebird\FirebirdGeneratorTests.cs" />
<Compile Include="Unit\Generators\Postgres\PostgresDataTests.cs" />
@@ -384,4 +385,4 @@
<Target Name="AfterBuild">
</Target>
-->
-</Project>
+</Project>
View
68 src/FluentMigrator.Tests/Unit/Expressions/UpdateDataExpressionTests.cs
@@ -0,0 +1,68 @@
+using System.Collections.Generic;
+using FluentMigrator.Expressions;
+using FluentMigrator.Infrastructure;
+using FluentMigrator.Tests.Helpers;
+using NUnit.Framework;
+using NUnit.Should;
+
+namespace FluentMigrator.Tests.Unit.Expressions
+{
+ [TestFixture]
+ public class UpdateDataExpressionTests {
+ private UpdateDataExpression expression;
+
+ [SetUp]
+ public void Initialize()
+ {
+ expression =
+ new UpdateDataExpression()
+ {
+ TableName = "ExampleTable",
+ Set = new List<KeyValuePair<string, object>>
+ {
+ new KeyValuePair<string, object>("Column", "value")
+ },
+ IsAllRows = false
+ };
+ }
+
+ [Test]
+ public void NullUpdateTargetCausesErrorMessage()
+ {
+ // null is the default value, but it might not always be, so I'm codifying it here anyway
+ expression.Where = null;
+
+ var errors = ValidationHelper.CollectErrors(expression);
+ errors.ShouldContain(ErrorMessages.UpdateDataExpressionMustSpecifyWhereClauseOrAllRows);
+ }
+
+ [Test]
+ public void EmptyUpdateTargetCausesErrorMessage()
+ {
+ // The same should be true for an empty list
+ expression.Where = new List<KeyValuePair<string, object>>();
+
+ var errors = ValidationHelper.CollectErrors(expression);
+ errors.ShouldContain(ErrorMessages.UpdateDataExpressionMustSpecifyWhereClauseOrAllRows);
+ }
+
+ [Test]
+ public void DoesNotRequireWhereConditionWhenIsAllRowsIsSet()
+ {
+ expression.IsAllRows = true;
+
+ var errors = ValidationHelper.CollectErrors(expression);
+ errors.ShouldNotContain(ErrorMessages.UpdateDataExpressionMustSpecifyWhereClauseOrAllRows);
+ }
+
+ [Test]
+ public void DoesNotAllowWhereConditionWhenIsAllRowsIsSet()
+ {
+ expression.IsAllRows = true;
+ expression.Where = new List<KeyValuePair<string, object>> {new KeyValuePair<string, object>("key", "value")};
+
+ var errors = ValidationHelper.CollectErrors(expression);
+ errors.ShouldContain(ErrorMessages.UpdateDataExpressionMustNotSpecifyBothWhereClauseAndAllRows);
+ }
+ }
+}
View
6 src/FluentMigrator/Expressions/UpdateDataExpression.cs
@@ -35,6 +35,12 @@ public override void CollectValidationErrors(ICollection<string> errors)
{
if (String.IsNullOrEmpty(TableName))
errors.Add(ErrorMessages.TableNameCannotBeNullOrEmpty);
+
+ if (!IsAllRows && (Where == null || Where.Count == 0))
+ errors.Add(ErrorMessages.UpdateDataExpressionMustSpecifyWhereClauseOrAllRows);
+
+ if (IsAllRows && Where != null && Where.Count > 0)
+ errors.Add(ErrorMessages.UpdateDataExpressionMustNotSpecifyBothWhereClauseAndAllRows);
}
public override void ExecuteWith(IMigrationProcessor processor)
View
2  src/FluentMigrator/Infrastructure/ErrorMessages.cs
@@ -43,5 +43,7 @@ public static class ErrorMessages
public const string OperationCannotBeNull = "The operation to be performed using the database connection cannot be null";
public const string DestinationSchemaCannotBeNull = "The destination schema's name cannot be null or an empty string";
public const string SequenceNameCannotBeNullOrEmpty = "The sequence's name cannot be null or an empty string";
+ public const string UpdateDataExpressionMustSpecifyWhereClauseOrAllRows = "Update statement is missing a condition. Specify one by calling .Where() or target all rows by calling .AllRows().";
+ public const string UpdateDataExpressionMustNotSpecifyBothWhereClauseAndAllRows = "Update statement specifies both a .Where() condition and that .AllRows() should be targeted. Specify one or the other, but not both.";
}
}
Something went wrong with that request. Please try again.