SetError

[marshall-mcmullen]

Is there a way to set an error on a SelectDataset (or any other dataset for that matter) ? It would be super convenient if my code which is building up a query using goqu could leverage its internal error tracking so a subsequent ToSQL would give me back the error I set (or any other internal goqu error) instead of me having to add another layer on top of goqu to track this.

Here's some pseudocode example of what I'd like to be able to do:

var sel *goqu.SelectDataset = ....
if (filter != Equal) {
    return sel.SetError(fmt.Errorf("Only Equal operation is supported"))
}

Then, later on when I go to execute the resulting SQL, this would provide a really nice idiomatic way to detect and propagate the error:

    query, args, err := sqlizer.ToSQL()
    if err != nil {
        return err
    }   

Thoughts? Better ideas on how to handle this ?

Thanks!!

[doug-martin]

Not currently but I can see how it would be useful. I think we should add it to all the dataset types to keep consistency.

[marshall-mcmullen]

@doug-martin I'd like to try to get this working in a PR. I started what I thought would be pretty straightforward by doing this:

// Get any error that has been set on the underlying SQLBuilder or nil if not error has been set.
func (dd *DeleteDataset) Error() error {
    return dd.deleteSQLBuilder().Error()
}

// Set an error on the underlying SQLBuilder. This error will be returned by a future call to Error or as part of ToSQL.
// This can be used by end users to record errors while building up queries without having to track those separately.
func (dd *DeleteDataset) SetError(err error) *DeleteDataset {
    return dd.deleteSQLBuilder().SetError(err)
}   

For all the dataset types. The Error() compiles fine. But no joy on SetError. Instead I get:

# github.com/doug-martin/goqu/v8
./delete_dataset.go:181:39: cannot use dd.deleteSQLBuilder().SetError(err) (type sb.SQLBuilder) as type *DeleteDataset in return argument

So I tried a type-assertion:

// Set an error on the underlying SQLBuilder. This error will be returned by a future call to Error or as part of ToSQL.
// This can be used by end users to record errors while building up queries without having to track those separately.
func (dd *DeleteDataset) SetError(err error) *DeleteDataset {
    return dd.deleteSQLBuilder().SetError(err).(*DeleteDataset)                                                                                   
}

Which causes more type problems:

# github.com/doug-martin/goqu/v8
./delete_dataset.go:181:44: impossible type assertion:
	*DeleteDataset does not implement sb.SQLBuilder (missing CurrentArgPosition method)

I'm clearly doing this wrong. Do you see a better way to implement SetError or is this just going to take more digging.

[marshall-mcmullen]

I almost think that Error and SetError need to get moved out of sql_builder.go and up into each dataset. That will lead to code duplication though. Perhaps there should be a common Dataset interface to avoid the code duplication...

[doug-martin]

I think a solution that may work, to keep changes to a minimum, could be to add a SetError method to each Dataset, and in the sqlBuilder method set the error if it exists on the dataset. So for DeleteDataset the changes could be.

type DeleteDataset struct {
	dialect      SQLDialect
	clauses      exp.DeleteClauses
	isPrepared   bool
	queryFactory exec.QueryFactory
	err          error
}

// used interally to copy the dataset
func (dd *DeleteDataset) copy(clauses exp.DeleteClauses) *DeleteDataset {
	return &DeleteDataset{
		dialect:      dd.dialect,
		clauses:      clauses,
		isPrepared:   dd.isPrepared,
		queryFactory: dd.queryFactory,
		err:          dd.err,
	}
}

func (dd *DeleteDataset) deleteSQLBuilder() sb.SQLBuilder {
	buf := sb.NewSQLBuilder(dd.isPrepared)
	if dd.err != nil {
		return buf.SetError(dd.err)
	}
	dd.dialect.ToDeleteSQL(buf, dd.clauses)
	return buf
}

The same pattern would be followed for the other datasets. If you can think of a way to abstract that logic out go for it.

Hope this helps.

[doug-martin]

If you are going to work this I'd like to get it in before merging #151, since it could be part v8. Let me know your thoughts.

[marshall-mcmullen]

If you are going to work this I'd like to get it in before merging #151, since it could be part v8. Let me know your thoughts.

I agree. I think the suggestion you have will work great. I think I can have something ready tomorrow. Then we can work on getting in #151 afterwards.

Thanks so much for your help, it's been very appreciated!

[marshall-mcmullen]

@doug-martin I've gone with the suggestion you gave me and created a PR: #152

There's a fair bit of code duplication but without some major refactoring I didn't see a simple way around that. And I wanted to keep this change simple.

I did add some tests around the new behavior as well. Please let me know what you think.

[doug-martin]

@marshallmcmullen just released this, Great job!

[marshall-mcmullen]

Woohoo! Great news. Thanks for helping me through my first goqu PR @doug-martin !