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

loadObject should return unique result #784

Merged
merged 2 commits into from
Aug 17, 2021
Merged

loadObject should return unique result #784

merged 2 commits into from
Aug 17, 2021

Conversation

wcekan
Copy link
Contributor

@wcekan wcekan commented Apr 11, 2019

If the default datastore loadObject fails to filter to a single result, do not return any.

Currently the first element of the collection would erroneously be returned.

@coveralls
Copy link
Collaborator

Coverage Status

Coverage decreased (-0.04%) to 75.902% when pulling 815cbec on defaultDataStore into c9b1567 on master.

return it.next();
Object obj = it.next();
if (!it.hasNext()) {
return obj;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this fix an actual issue?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe throw an exception instead of returning null.

Copy link
Contributor Author

@wcekan wcekan Apr 15, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The contract is for loadObject to return null if no object is loaded. But perhaps we can throw an exception when too many are returned.

return it.next();
Object obj = it.next();
if (!it.hasNext()) {
return obj;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe throw an exception instead of returning null.

@aklish aklish merged commit 0592714 into master Aug 17, 2021
@aklish aklish deleted the defaultDataStore branch August 17, 2021 15:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants