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

Reject conflicting updates #2582

Merged

Conversation

schmidt-sebastian
Copy link
Contributor

@schmidt-sebastian schmidt-sebastian commented Nov 6, 2017

This rejects updates that include ambiguous field definitions, such as "a.b" : "foo" and "a" : "foo"

Note that this includes the same google-java-format changes as #2565

@googlebot googlebot added the cla: yes This human has signed the Contributor License Agreement. label Nov 6, 2017

@Override
public int compareTo(@Nonnull FieldPath other) {
return canonicalString().compareTo(other.canonicalString());

This comment was marked as spam.

This comment was marked as spam.

@pongad
Copy link
Contributor

pongad commented Nov 7, 2017

Oh right, I forgot we had this variable.
I just realized; the cache isn't thread-safe right? Do we want to use AutoValue's Memoize annotation?
If this is not a concern LGTM; we can also fix this in another PR.

@schmidt-sebastian
Copy link
Contributor Author

Oh right, I forgot we had this variable.
I just realized; the cache isn't thread-safe right? Do we want to use AutoValue's Memoize annotation?
If this is not a concern LGTM; we can also fix this in another PR.

Yes, this is indeed not thread-safe. I have the 'Memoize' annotation ready to go, but it requires us to update our dependency on AutoValue. We are currently on 1.2, while the release is up to 1.5.2 (which includes Memoize). Is that feasible?

@pongad
Copy link
Contributor

pongad commented Nov 7, 2017

Ah OK. I'm leaning towards updating AutoValue in its own PR. Looking through release notes, there are some changes that might break things. I'm OK with updating AutoValue version and adding Memoize in the same PR though.

Do you have cycles to pick this up?

@pongad
Copy link
Contributor

pongad commented Nov 7, 2017

@garrettjonesgoogle Do you also want to take a look? LGTM from me, so feel free to just merge if you think your review isn't necessary.

@garrettjonesgoogle
Copy link
Member

We're trying to do a release, let's hold of on merging until that's done.

@schmidt-sebastian
Copy link
Contributor Author

Do you have cycles to pick this up?

I can update Firestore, but I don't have cycles to update the rest of the repo.

@schmidt-sebastian
Copy link
Contributor Author

As discussed offline with @pongad, this is good to merge.

@schmidt-sebastian schmidt-sebastian merged commit 7c93395 into googleapis:master Nov 10, 2017
schmidt-sebastian added a commit that referenced this pull request Nov 10, 2017
schmidt-sebastian added a commit that referenced this pull request Nov 10, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes This human has signed the Contributor License Agreement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants