Separate idea of Expression from Operand #1476

hhayano · 2017-08-16T19:06:07Z

For changes to files under the /model/ folder, and manual edits to autogenerated code (e.g. /service/s3/api.go) please create an Issue instead of a PR for those type of changes.

If there is an existing bug or feature this PR is answers please reference it here.

…tring constants

xibz · 2017-08-17T21:20:42Z

service/dynamodb/expression/expression.go

+		// TODO consider AST instead of string in the future
+		switch expr.Expression[i+1] {
+		case 'p':
+			if index.name >= len(en.names) {


How about putting each of these cases in their own functions?

xibz · 2017-08-17T21:28:23Z

service/dynamodb/expression/expression.go

+	for _, expr := range list {
+		for alias, name := range expr.Names {
+			if ret.Names == nil {
+				ret.Names = make(map[string]*string)


minor, but this could be ret.Names = map[string]*string{}

…eturning

jasdel · 2017-08-24T23:06:44Z

service/dynamodb/expression/expression.go

+//       ExpressionAttributeValues: factory.Values(),
+//       TableName: aws.String("SomeTable"),
+//     }
+func (factory Factory) Projection() *string {


Suggest instead of using method for each expression type use a single getter taking the expression type as input.

https://play.golang.org/p/txrOvLujQW

jasdel · 2017-08-24T23:30:23Z

service/dynamodb/expression/condition.go

-//     expression, err := condition.BuildExpression() // Used to make an Expression
-func BeginsWith(p PathBuilder, s string) ConditionBuilder {
+//     factoryBuilder := Condition(condition)         // Used to make an FactoryBuilder
+func BeginsWith(nameBuilder NameBuilder, substr string) ConditionBuilder {


suggest substr be prefix

jasdel · 2017-08-24T23:48:20Z

service/dynamodb/expression/operand.go

 type OperandBuilder interface {
-	BuildOperand() (ExprNode, error)
+	BuildNode() (ExprNode, error)


Suggest this be a BuildOperand which returns a Operand type which could be something like the following.

type Operand struct { Name string Value dynamodb.AttributeValue Expression string }

This will allow you to make ExprNode private,

jasdel · 2017-08-24T23:48:54Z

service/dynamodb/expression/condition.go

+// BuildTree() method will only be called recursively by the functions
+// BuildFactory and buildChildTrees. This function should not be called by the
+// users.
+func (conditionBuilder ConditionBuilder) BuildTree() (ExprNode, error) {


This can be private since TreeBuilder can be a private internal interface.

jasdel · 2017-08-24T23:55:50Z

service/dynamodb/expression/expression.go

+//       ExpressionAttributeValues: factory.Values(),
+//       TableName: aws.String("SomeTable"),
+//     }
+type FactoryBuilder struct {


Suggest instead of a factor builder being constructed by things like Condition switch it around and define an ExpressionBuilder which as helper methods to add Condition, Update ect.

type Builder struct {} func NewBuilder() Builder {} func (e Builder) WithCondition(builder ConditionBuilder) ExpressionBuilder {} func (e Builder) WithUpdate(builder UpdateBuilder) ExpressionBuilder {} ... func (e Builder) Build() Expression {} type Expression struct {} func (e Expression) Names() map[string]*string {} func (e Expression) Values() map[string]*dynamodb.AttributeValue {} func (e Expression) Get(expType Type) *string {} // or func (e Expression) Condition() *string {}

jasdel

Few general comments

Error handling: inconsistent experience for predefined awserr.Error vs fmt.Errorf errors. Should be consistent.
Error wording: the function the error occurred in generally is not as important to the user as the error that occurred. The wording should be more specific from the user's point of view. e.g. build condition failed, unknown condition mode xzy

jasdel · 2017-08-28T16:54:05Z

service/dynamodb/expression/condition.go

 	default:
-		return ExprNode{}, fmt.Errorf("buildCondition error: unsupported mode: %v", cond.mode)
+		return exprNode{}, fmt.Errorf("buildTree error: unsupported mode: %v", cb.mode)


Should be consistent with errors that are returned. in the unsetCond case a awserr.Error is used but generic error is used here.

For errors can consider what action the receiver is expected to take, or do with the error. In most cases (all?) these errors are fatal, and inform the user that their using invalid input. Because of this the awserr.Error may be overly onerous.

Also consider that the buildTree part of the error message isn't helpful to a user. What about error messages something like, "build condition failed, unsupported mode %v", cb.mode

jasdel · 2017-08-28T16:55:11Z

service/dynamodb/expression/condition.go

+	case greaterThanCond:
+		node.fmtExpr = "$c > $c"
+	case greaterThanEqualCond:
+		node.fmtExpr = "$c >= $c"
 	}


probably want error default case for known condition mode. This will also help adding additional cases in the future.

jasdel · 2017-08-28T16:56:05Z

service/dynamodb/expression/condition.go

 	// create a string with escaped characters to substitute them with proper
 	// aliases during runtime
 	var mode string
-	switch c.mode {
+	switch conditionBuilder.mode {
 	case andCond:
 		mode = " AND "
 	case orCond:
 		mode = " OR "


probably want default case also.

jasdel · 2017-08-28T18:25:30Z

service/dynamodb/expression/expression.go

+
+// Condition will return the *string corresponding to the Condition Expression
+// of the argument Expression. This method is used to satisfy the members of
+// DynamoDB input structs. If the argument Expression does not have a


nit, Expression here generally isn't talked about as argument. In this context it is more similar to a type that has methods on it. e.g If the Expression does not have a condition expression this method will return nil.

jasdel · 2017-08-28T18:28:32Z

service/dynamodb/expression/expression.go

+//       TableName: aws.String("SomeTable"),
+//     }
+func (e Expression) Names() map[string]*string {
+	aliasList, _ := e.buildChildTrees()


This should be done inside of Builder.Build so that any errors can be surfaced to the user. In addition, this will reduce the duplicate logic that is occurring in each getter. I think it makes sense to see the Expression type as a read only view into the already built/constructed data created by Builder

jasdel · 2017-08-28T18:37:01Z

service/dynamodb/expression/expression.go

+
+	valuesMap := map[string]*dynamodb.AttributeValue{}
+	for i := 0; i < len(aliasList.valuesList); i++ {
+		valuesMap[fmt.Sprintf(":%v", i)] = &aliasList.valuesList[i]


This logic should be apart of the Builder.Build method.

jasdel · 2017-08-28T18:37:46Z

service/dynamodb/expression/expression.go

+
+	namesMap := map[string]*string{}
+	for ind, val := range aliasList.namesList {
+		namesMap[fmt.Sprintf("#%v", ind)] = aws.String(val)


This logic should be apart of the Builder.Build method.

jasdel · 2017-08-28T18:45:36Z

service/dynamodb/expression/operand.go

-// PathBuilder represents a path to either a top level item attribute or a
-// nested attribute. It will implement the OperandBuilder interface. It will
+// NameBuilder represents a name of a top level item attribute or a nested
+// attribute. It will implement the OperandBuilder interface. It will
 // have various methods corresponding to the operations supported by DynamoDB
 // operations. (i.e. AND, BETWEEN, EQUALS)


Should state that a Name is a DynamoDB Operand to create that clear relationship.

jasdel

👍

reviewed by jasdel

hhayano added 7 commits August 16, 2017 11:17

initial commit to separate expression from operand

e687446

initial commit to separate expression from operand

b742f85

separate idea of expression from operand and make MergeMaps exported

866cf57

Improve tests

a192e4f

Improve comment grammar, get rid of iota constants and replace with s…

aec4b2d

…tring constants

change go build for tests to 1.7

ea18d40

Add comment about Filter Expression

34b4ecd

xibz previously requested changes Aug 17, 2017

View reviewed changes

hhayano added 5 commits August 17, 2017 15:05

create separate functions for alias switch function

c617572

Start implementing Expression methods

e1e8f7d

Implement TreeBuilder and methods for Expression. TODO refine error r…

f36d3c8

…eturning

TODO implement factory

e2ae2c1

Implement FactoryBuilder and Factory

32314a9

jasdel suggested changes Aug 25, 2017

View reviewed changes

hhayano added 3 commits August 25, 2017 13:03

Implement requested changes

7cd7356

Implement the function NewBuilder()

c360025

Return nil for empty names and values

4e0e47c

jasdel suggested changes Aug 28, 2017

View reviewed changes

hhayano added 3 commits August 28, 2017 13:19

Improve comments, move logic of build to the Build method for Builder

b7511cc

Implement typed error structs

9c6dd56

add comments to the typed error structs

d0dbabd

jasdel approved these changes Aug 30, 2017

View reviewed changes

jasdel merged commit ce236ed into aws:dynamodb-abstraction Aug 30, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Separate idea of Expression from Operand #1476

Separate idea of Expression from Operand #1476

hhayano commented Aug 16, 2017

xibz Aug 17, 2017

xibz Aug 17, 2017

jasdel Aug 24, 2017

jasdel Aug 24, 2017

jasdel Aug 24, 2017

jasdel Aug 24, 2017

jasdel Aug 24, 2017

jasdel left a comment

jasdel Aug 28, 2017

jasdel Aug 28, 2017

jasdel Aug 28, 2017

jasdel Aug 28, 2017

jasdel Aug 28, 2017

jasdel Aug 28, 2017

jasdel Aug 28, 2017

jasdel Aug 28, 2017

jasdel left a comment

Separate idea of Expression from Operand #1476

Separate idea of Expression from Operand #1476

Conversation

hhayano commented Aug 16, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jasdel left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jasdel left a comment

Choose a reason for hiding this comment