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

Possible bug in Family.Builder #148

Closed
Lusito opened this issue Mar 10, 2015 · 4 comments
Closed

Possible bug in Family.Builder #148

Lusito opened this issue Mar 10, 2015 · 4 comments
Labels

Comments

@Lusito
Copy link
Contributor

Lusito commented Mar 10, 2015

I was just looking through the changelog and noticed this commit:
a9b3eea

The call for reset() was moved from before creating a Family to after creating the Family.
Imagine a setup like this:

Family family = Family.all(getAll()).one(getOne()).get();

Now if getOne() throws an exception, Builder.all will not be reset, creating a bad family on the next call to Family.all|one|exclude.

In a case like #137, which instantiates its own Family.Builder Object (which should not be needed in most cases), there still is no need for this:
If you want to reuse the custom builder instance after get(), just call reset() on it manually.

Having reset() called inside of get() also causes another problem in this situation:

Builder builder = Family.all(x);
Family a = builder.get();
Family b = builder.one(y).get(); // you'd expect b == Family.all(x).one(y).get(), but you'll get b == Family.one(y).get();
@54k
Copy link

54k commented Mar 11, 2015

I think the simplest approach is just creating new builder in all static methods, like artemis odb does. Or document "reset" behavior. I think the first solution is more predictable.

@Lusito
Copy link
Contributor Author

Lusito commented Mar 11, 2015

I'd say it's up to the user. If the user wants a unique instance, he can just create a new builder.
In most cases you can reuse the single instance.

What exactly do you mean by document reset behavior ?

@54k
Copy link

54k commented Mar 11, 2015

I mean to add a note about reset feature into the builder's get method javadoc. Sorry for engrish.

Upd: anyway I think that the shared builder is unnecessary. And leads to bugs that you described.

@Lusito
Copy link
Contributor Author

Lusito commented Mar 11, 2015

ah, documentation, I thought about a document ^^, yes sure.

I think wasting a builder every time you want a family is unnecessary:
new Builder().all(...).one(..).get() seems wasteful to me.

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

No branches or pull requests

3 participants