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

SelectDataset From with Error #183

Closed
1 of 3 tasks
marshall-mcmullen opened this issue Nov 27, 2019 · 2 comments
Closed
1 of 3 tasks

SelectDataset From with Error #183

marshall-mcmullen opened this issue Nov 27, 2019 · 2 comments
Milestone

Comments

@marshall-mcmullen
Copy link
Contributor

marshall-mcmullen commented Nov 27, 2019

Describe the bug
If you use SelectDataset From and pass it another SelectDataset that has an error already set on it, it seems to ignore the error on the subselect instead of propagating it.

To Reproduce

func TestSelectWithSubSelectError(t *testing.T) {

    ds := goqu.From("test").As("t").SetError(fmt.Errorf("An error"))
    assert.NotNil(t, ds.Error())

    ds2 := goqu.From(ds)                                                                                 
    assert.NotNil(t, ds2.Error())
}

If you run this test the first assertion passes, but the second assertion fails.

        	Error:      	Expected value not to be nil.
        	Test:       	TestSelectWithSubSelectError

Expected behavior
I would expect using a SelectDataset as a subselect on another SelectDataset would honor the error and propagate it rather than swallow the error. Obviously the caller can check this scenario with gs.Error() before using it in a subselect, but I think that puts too much burden on the caller IMO and this is something goqu should handle.

Dialect:

  • postgres
  • mysql
  • sqlite3

Additional context
Add any other context about the problem here.

@marshall-mcmullen
Copy link
Contributor Author

I've created a PR: #184 to address this issue in a way that made sense to me. @doug-martin please let me know what you think.

doug-martin added a commit that referenced this issue Dec 7, 2019
* Changed AppendSQL to set error if one is present
doug-martin added a commit that referenced this issue Dec 7, 2019
* Changed AppendSQL to set error if one is present
doug-martin added a commit that referenced this issue Dec 7, 2019
@doug-martin doug-martin mentioned this issue Dec 7, 2019
@doug-martin doug-martin added this to the v9.5.1 milestone Dec 7, 2019
@marshall-mcmullen
Copy link
Contributor Author

Thanks so much for getting this merged @doug-martin

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants