I will review the details shortly, but I would like to start by saying that I
think package com.google.api.client.extensions.jdo.auth.oauth2 doesn't belong in
the google-oauth-client-servlet artifact. You can tell from the package naming
that I hadn't thought of it as being servlet specific. Please create a new
artifact google-oauth-client-jdo and move this package there. The good news is
that there's no need to rename the package or deprecate the existing class
because a move from one artifact to another should be considered a sufficiently
backwards-compatible change as long as we keep things named the same.
https://codereview.appspot.com/7575043/diff/9012/google-oauth-client-jdo/src/main/java/com/google/api/client/extensions/jdo/auth/oauth2/JdoCredentialStore.java File google-oauth-client-jdo/src/main/java/com/google/api/client/extensions/jdo/auth/oauth2/JdoCredentialStore.java (right): https://codereview.appspot.com/7575043/diff/9012/google-oauth-client-jdo/src/main/java/com/google/api/client/extensions/jdo/auth/oauth2/JdoCredentialStore.java#newcode2 google-oauth-client-jdo/src/main/java/com/google/api/client/extensions/jdo/auth/oauth2/JdoCredentialStore.java:2: * Copyright (c) 2012 Google Inc. For some reason ...
https://codereview.appspot.com/7575043/diff/14001/google-oauth-client/src/mai...
File
google-oauth-client/src/main/java/com/google/api/client/auth/oauth2/CredentialStore.java
(right):
https://codereview.appspot.com/7575043/diff/14001/google-oauth-client/src/mai...
google-oauth-client/src/main/java/com/google/api/client/auth/oauth2/CredentialStore.java:72:
void delete(String userId) throws IOException;
I'm having second thoughts about changing the interface. Maybe this is a crazy
idea, but I think it has a lot of potential. We could make an abstract Store
interface in the http project that allows for arbitrary data storage based on a
String key and Serializable data. We would then have implementations like
MemoryStore, JdoStore, and AppEngineStore, and make them all thread safe.
In addition to providing a great long-term solution, this would also hopefully
solve some immediate problems:
1. We have a lot of duplicate code between the implementations of
CredentialStore and SubscriptionStore (designed for storing Push Notifications
in the API project).
2. Currently Credential stores only 3 fields, and in the future we may need to
store other fields. Specifically, we may want to store id_token. Additionally,
developers may want to store their own custom data per user.
3. MemoryCredentialStore, JdoCredentialStore, and AppEngineCredentialStore all
need work anyway, and potentially in slightly incompatible ways. We could
instead simply deprecate them and ask that the new general-purpose Store be
used.
What do you think Eyal? If we agree, are you interested in owning this work?
Eyal, if you agree with my proposal, I propose that for 1.15 we do the bare
minimum the fix just the immediate bug, and not change the CredentialStore
interface or split off a new google-oauth-client-jdo project. I would suggest
also that we consider adding the @Beta annotation to CredentialStore with the
possibility of eventually removing that interface.
https://codereview.appspot.com/7575043/diff/14001/google-oauth-client-jdo/.cl...
File google-oauth-client-jdo/.classpath (right):
https://codereview.appspot.com/7575043/diff/14001/google-oauth-client-jdo/.cl...
google-oauth-client-jdo/.classpath:4: <classpathentry kind="con"
path="org.eclipse.jdt.launching.JRE_CONTAINER/org.eclipse.jdt.internal.debug.ui.launcher.StandardVMType/J2SE-1.5"/>
On 2013/04/19 19:59:50, yanivi wrote:
> change this to:
> <classpathentry kind="con" path="org.eclipse.jdt.launching.JRE_CONTAINER"/>
Irrelevant now when removing google-oauth-client-jdo artifact
https://codereview.appspot.com/7575043/diff/14001/google-oauth-client-jdo/.pr...
File google-oauth-client-jdo/.project (right):
https://codereview.appspot.com/7575043/diff/14001/google-oauth-client-jdo/.pr...
google-oauth-client-jdo/.project:4: <comment>Internally developed code released
as open source. NO_M2ECLIPSE_SUPPORT: Project files created with the
maven-eclipse-plugin are not supported in M2Eclipse.</comment>
On 2013/04/19 19:59:50, yanivi wrote:
> <comment></comment>
Irrelevant now when removing google-oauth-client-jdo artifact
https://codereview.appspot.com/7575043/diff/14001/google-oauth-client-jdo/.pr...
google-oauth-client-jdo/.project:7: <buildSpec>
On 2013/04/19 19:59:50, yanivi wrote:
> tabs not spaces
Irrelevant now when removing google-oauth-client-jdo artifact
https://codereview.appspot.com/7575043/diff/14001/google-oauth-client-jdo/pom...
File google-oauth-client-jdo/pom.xml (right):
https://codereview.appspot.com/7575043/diff/14001/google-oauth-client-jdo/pom...
google-oauth-client-jdo/pom.xml:19:
<link>http://code.google.com/appengine/docs/java/javadoc</link>
On 2013/04/19 19:59:50, yanivi wrote:
> remove this line
>
> similarly for google-oauth-client-servlet
Irrelevant now when removing google-oauth-client-jdo artifact
https://codereview.appspot.com/7575043/diff/14001/google-oauth-client-jdo/src...
File
google-oauth-client-jdo/src/main/java/com/google/api/client/extensions/jdo/auth/oauth2/JdoCredentialStore.java
(right):
https://codereview.appspot.com/7575043/diff/14001/google-oauth-client-jdo/src...
google-oauth-client-jdo/src/main/java/com/google/api/client/extensions/jdo/auth/oauth2/JdoCredentialStore.java:48:
persistedCredential =
It will only save us one line of code, so I prefer to leave it as it is. I don't
see any benefits from that....
On 2013/04/19 19:59:50, yanivi wrote:
> [optional] add utility method with implementation of "return
> persistenceManager.getObjectById(JdoPersistedCredential.class, userId);" to be
> shared between these 3 methods
https://codereview.appspot.com/7575043/diff/14001/google-oauth-client-jdo/src...
google-oauth-client-jdo/src/main/java/com/google/api/client/extensions/jdo/auth/oauth2/JdoCredentialStore.java:54:
if (persistedCredential == null) {
On 2013/04/19 19:59:50, yanivi wrote:
> I find this logic more confusing than needed. Why not just something more
like
> this:
>
> JdoPersistedCredential persistedCredential;
> try {
> persistedCredential =
> persistenceManager.getObjectById(JdoPersistedCredential.class, userId);
> persistedCredential.update(credential);
> } catch (JDOObjectNotFoundException e) {
> persistedCredential = new JdoPersistedCredential(userId, credential);
> persistenceManager.makePersistent(persistedCredential);
> } finally {
> persistenceManager.close();
> }
Done.
https://codereview.appspot.com/7575043/diff/14001/google-oauth-client/src/mai...
File
google-oauth-client/src/main/java/com/google/api/client/auth/oauth2/CredentialStore.java
(right):
https://codereview.appspot.com/7575043/diff/14001/google-oauth-client/src/mai...
google-oauth-client/src/main/java/com/google/api/client/auth/oauth2/CredentialStore.java:72:
void delete(String userId) throws IOException;
We agreed on that for now I'll just annotate StoreCredential as Beta, and delete
google-oauth-client-jdo aritfact.
On 2013/04/20 13:29:30, yanivi wrote:
> I'm having second thoughts about changing the interface. Maybe this is a
crazy
> idea, but I think it has a lot of potential. We could make an abstract Store
> interface in the http project that allows for arbitrary data storage based on
a
> String key and Serializable data. We would then have implementations like
> MemoryStore, JdoStore, and AppEngineStore, and make them all thread safe.
>
> In addition to providing a great long-term solution, this would also hopefully
> solve some immediate problems:
>
> 1. We have a lot of duplicate code between the implementations of
> CredentialStore and SubscriptionStore (designed for storing Push Notifications
> in the API project).
>
> 2. Currently Credential stores only 3 fields, and in the future we may need to
> store other fields. Specifically, we may want to store id_token.
Additionally,
> developers may want to store their own custom data per user.
>
> 3. MemoryCredentialStore, JdoCredentialStore, and AppEngineCredentialStore all
> need work anyway, and potentially in slightly incompatible ways. We could
> instead simply deprecate them and ask that the new general-purpose Store be
> used.
>
> What do you think Eyal? If we agree, are you interested in owning this work?
https://codereview.appspot.com/7575043/diff/14001/pom.xml
File pom.xml (right):
https://codereview.appspot.com/7575043/diff/14001/pom.xml#newcode66
pom.xml:66: * jdiff.xml
On 2013/04/19 19:59:50, yanivi wrote:
> please also update jdiff.xml file here and in api project
Irrelevant now after removing google-oauth-client-jdo artifact
https://codereview.appspot.com/7575043/diff/14001/pom.xml#newcode67
pom.xml:67: * release.html
On 2013/04/19 19:59:50, yanivi wrote:
> remove this line
>
> similarly for the http & api projects
Done.
https://codereview.appspot.com/7575043/diff/14001/pom.xml#newcode70
pom.xml:70: * google-oauth-client-assembly/readme.html
On 2013/04/19 19:59:50, yanivi wrote:
> update these files as well:
>
> google-oauth-client-assembly/readme.html
> google-api-client-assembly/readme.html
> google-oauth-client-assembly/classpath-include
> google-api-client-assembly/classpath-include
> google-oauth-client-assembly/pom.xml
Irrelevant now when removing google-oauth-client-jdo artifact
Issue 7575043: Issues 46 and 48: JdoCredentialStore - duplicate entry exception and transient instance delete
(Closed)
Created 11 years, 1 month ago by peleyal
Modified 11 years ago
Reviewers: yanivi
Base URL: https://code.google.com/p/google-oauth-java-client/
Comments: 22
