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

Guava 23 edgeValueOrDefault missing edge returns non-default value #2924

Closed
rocketraman opened this issue Aug 23, 2017 · 5 comments
Closed
Assignees
Labels
package=graph type=defect Bug, not working as expected
Milestone

Comments

@rocketraman
Copy link

rocketraman commented Aug 23, 2017

In Guava 23...

Assume nodeA exists but #nodeU does not exist. Execute:

graph.edgeValueOrDefault(nodeA, nodeU, 5)

The result is null. I would expect the result to be 5, given that there is no edge between nodeA and non-existent nodeU. Guava 22 an earlier have the expected behavior.

I see that Guava 23 now returns an Optional for edgeValue, so the statement can easily be converted to graph.edgeValue(nodeA, nodeU, 5).orElse(5), however if this is the intended behavior of the edgeValueOrDefault method for this situation, it deserves some additional explanation in the Javadoc. Or simply deprecate this method and recommend people transition to edgeValue().orElse().

@cpovirk cpovirk added package=graph type=defect Bug, not working as expected labels Aug 23, 2017
@cpovirk cpovirk added this to the 24.0 milestone Aug 23, 2017
@jbduncan
Copy link
Contributor

jbduncan commented Oct 4, 2017

Now that Guava is going to have more frequent releases and thus 23.2 is likely to be released before 24, I wonder if it would make sense to shift the milestone to 23.x.

@cpovirk, do you have any thoughts on this?

@cpovirk
Copy link
Member

cpovirk commented Oct 4, 2017

Thanks. Tentatively renamed to "NEXT." @ronshapiro @cgdecker may have more thoughts.

@cpovirk
Copy link
Member

cpovirk commented Oct 4, 2017

(And I moved the 2 closed issues that were tagged 24.0 to a new 23.1 milestone, since they both made it into that release.)

@jrtom
Copy link
Member

jrtom commented Oct 4, 2017

Somehow I completely failed to notice this issue previously; my apologies.

The behavior of edgeValueOrDefault() wasn't intended to change from 22.0 to 23.0; I introduced a regression when doing some internal refactoring that apparently wasn't caught by our tests. Sorry about that.

I'll get a fix out for that as soon as I can.

cpovirk pushed a commit that referenced this issue Oct 5, 2017
Also fixed tests to guard against similar future regressions.

(This fixes a regression that was introduced when we temporarily replaced this method with edgeValueOrNull() ([] and then failed to restore the original semantics when we restored the original method ([]  Oops.)

Ref: GitHub issue #2924

-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=171145586
@jrtom
Copy link
Member

jrtom commented Oct 5, 2017

Fixed (see above commit). Thanks, @rocketraman.

@jrtom jrtom closed this as completed Oct 5, 2017
cpovirk pushed a commit that referenced this issue Oct 11, 2017
…y exercising both null and non-null default values.

This addresses the most recent suggestion in GitHub issue #2924.

-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=171818169
@cgdecker cgdecker modified the milestones: NEXT, 23.2 Oct 11, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
package=graph type=defect Bug, not working as expected
Projects
None yet
Development

No branches or pull requests

5 participants