Issue 9618044: Add support for Heterogeneous Schemata (Closed)
DescriptionAdd support for Heterogeneous Schemata
https://code.google.com/p/google-http-java-client/issues/detail?id=173
Patch Set 1 #Patch Set 2 : Added a working implementation #Patch Set 3 : Fix to JSON string bug, add test case #
Total comments: 4
Patch Set 4 : Move logic into already-existing private methods #
Total comments: 15
Patch Set 5 : Comments; handle GenericJson #
Total comments: 40
Patch Set 6 : More comments and tweaks #Patch Set 7 : #Patch Set 8 : tiny #
Total comments: 2
Patch Set 9 : Review improvements #Patch Set 10 : Javadoc & formatting #
Total comments: 30
Patch Set 11 : Review improvements #Patch Set 12 : Parse all to GenericJson until type known, allow abstract, other improvements #
Total comments: 43
Patch Set 13 : More review feedback! #Patch Set 14 : Formatting #
MessagesTotal messages: 16
Great progress Nick! A few important comments to help guide you about things that will eventually need to be tackled, but don't need to be done immediately while you are still hacking and testing out the logic for polymorphic types. https://codereview.appspot.com/9618044/diff/5001/google-http-client/src/main/... File google-http-client/src/main/java/com/google/api/client/json/JsonParser.java (right): https://codereview.appspot.com/9618044/diff/5001/google-http-client/src/main/... google-http-client/src/main/java/com/google/api/client/json/JsonParser.java:51: private static Field CACHED_TYPEMAP_FIELD = null; CACHED_TYPEMAP_FIELD should probably instead be a Map from Class to Field. Better yet, we probably need a JsonClassInfo and JsonFieldInfo that work much like ClassInfo and FieldInfo (and don't duplicate anything that they manage), but add some JSON-specific stuff like JsonTypeMap and JsonString related info. https://codereview.appspot.com/9618044/diff/5001/google-http-client/src/main/... google-http-client/src/main/java/com/google/api/client/json/JsonParser.java:303: private class CaptureUnusedJsonParser extends CustomizeJsonParser { I don't think the use of CustomizeJsonParser is necessary. See below. https://codereview.appspot.com/9618044/diff/5001/google-http-client/src/main/... google-http-client/src/main/java/com/google/api/client/json/JsonParser.java:325: public final <T> T parsePolymorphic(Class<T> destinationClass) Let's not add a parsePolymorphic method. parseValue should have all of this logic inside it, so you can call the already existing parse methods on a class regardless of whether it is polymorphic. https://codereview.appspot.com/9618044/diff/5001/google-http-client/src/main/... google-http-client/src/main/java/com/google/api/client/json/JsonParser.java:349: T dumbParsed = parse(destinationClass, customizeParser); Don't call parse(destinationClass, customizeParser). Just call the internal method parseValue(null, destinationClass, new ArrayList<Type>(), null, null). But then we probably need to add a new parameter to parseValue to keep track of the unrecognized fields. Fortunately, it's easy to do so for an internal method, whereas we cannot do that for a public method.
Sign in to reply to this message.
I took your comments into consideration and moved the logic into the parseValue and parse private methods. You can now call the regular parse method on a heterogeneous schema and it works quite nicely. I added a test case where both the main object and one of its Fields are both polymorphic data types to confirm it handles both properly. Still a work in progress, but I'd appreciate any additional feedback on the new implementation. Thanks!
Sign in to reply to this message.
Good progress. Sorry I don't have time to investigate any more carefully than this, but hopefully these comments should help a bit. https://codereview.appspot.com/9618044/diff/14001/google-http-client/src/main... File google-http-client/src/main/java/com/google/api/client/json/JsonParser.java (right): https://codereview.appspot.com/9618044/diff/14001/google-http-client/src/main... google-http-client/src/main/java/com/google/api/client/json/JsonParser.java:296: String json; I think it would be better to stored the parsed JSON. See below. https://codereview.appspot.com/9618044/diff/14001/google-http-client/src/main... google-http-client/src/main/java/com/google/api/client/json/JsonParser.java:804: if (!cachedTypemapFields.containsKey(newInstanceClass)) { use locking to ensure thread-safety https://codereview.appspot.com/9618044/diff/14001/google-http-client/src/main... google-http-client/src/main/java/com/google/api/client/json/JsonParser.java:835: Field destinationField = typeClass.getField(unusedJson.key); I think this won't work in general because the field name doesn't necessarily match key name. Instead use ClassInfo.getField(unusedJson.key) https://codereview.appspot.com/9618044/diff/14001/google-http-client/src/main... google-http-client/src/main/java/com/google/api/client/json/JsonParser.java:837: getFactory().createJsonParser(unusedJson.json).parse(typeClass); why parse into typeClass? shouldn't it instead be the type of the field? next line makes no sense to me either. https://codereview.appspot.com/9618044/diff/14001/google-http-client/src/main... google-http-client/src/main/java/com/google/api/client/json/JsonParser.java:840: // This field isn't in the correct type either. Ignore it. Don't ignore it if we're parsing into something extending GenericJson, since that one preserves unknown JSON keys. The good news is that we've already parsed it, so if we keep the parsed JSON object we can just set to that directly. https://codereview.appspot.com/9618044/diff/14001/google-http-client/src/main... File google-http-client/src/main/java/com/google/api/client/json/Polymorphic.java (right): https://codereview.appspot.com/9618044/diff/14001/google-http-client/src/main... google-http-client/src/main/java/com/google/api/client/json/Polymorphic.java:28: public @interface Polymorphic { Do we want this annotation? The alternative is to check every loaded class for a field with JsonTypeMap annotation. It could be as simple to implement as using null Field value in cachedTypemapFields to indicate that we looked for such a field but didn't find it. https://codereview.appspot.com/9618044/diff/14001/google-http-client/src/main... File google-http-client/src/main/java/com/google/api/client/json/TypeDef.java (right): https://codereview.appspot.com/9618044/diff/14001/google-http-client/src/main... google-http-client/src/main/java/com/google/api/client/json/TypeDef.java:28: public @interface TypeDef { should we move @interface TypedDef inside JsonTypeMap.java like Jackson does with JsonSubTypes?
Sign in to reply to this message.
https://codereview.appspot.com/9618044/diff/14001/google-http-client/src/main... File google-http-client/src/main/java/com/google/api/client/json/JsonParser.java (right): https://codereview.appspot.com/9618044/diff/14001/google-http-client/src/main... google-http-client/src/main/java/com/google/api/client/json/JsonParser.java:804: if (!cachedTypemapFields.containsKey(newInstanceClass)) { On 2013/06/04 20:51:03, yanivi wrote: > use locking to ensure thread-safety Done. Eyal suggested I use double-checked locking here (http://www.ibm.com/developerworks/java/library/j-dcl/index.html) Still being a little fresh at concurrency in practice, I'd like to hear your opinion. https://codereview.appspot.com/9618044/diff/14001/google-http-client/src/main... google-http-client/src/main/java/com/google/api/client/json/JsonParser.java:835: Field destinationField = typeClass.getField(unusedJson.key); On 2013/06/04 20:51:03, yanivi wrote: > I think this won't work in general because the field name doesn't necessarily > match key name. Instead use ClassInfo.getField(unusedJson.key) Done. https://codereview.appspot.com/9618044/diff/14001/google-http-client/src/main... google-http-client/src/main/java/com/google/api/client/json/JsonParser.java:837: getFactory().createJsonParser(unusedJson.json).parse(typeClass); On 2013/06/04 20:51:03, yanivi wrote: > why parse into typeClass? shouldn't it instead be the type of the field? next > line makes no sense to me either. So to put it into concrete terms, we couldn't parse "tricksKnown" into Animal (which is why its a member of unusedJson) but we can parse it into Dog (which is typeClass). So we try to fit each bit of JSON that didn't match the parent type (Animal) into the specific type (Dog), and if it fits then we copy the value into our result object. While this works, I'm open to a cleaner suggestion. AFAIK, parsing into typeClass will not always work -- it fails for example parsing {"tricksKnown": 5} into an int, because it will try to call Types.newInstance on int. https://codereview.appspot.com/9618044/diff/14001/google-http-client/src/main... google-http-client/src/main/java/com/google/api/client/json/JsonParser.java:840: // This field isn't in the correct type either. Ignore it. On 2013/06/04 20:51:03, yanivi wrote: > Don't ignore it if we're parsing into something extending GenericJson, since > that one preserves unknown JSON keys. The good news is that we've already > parsed it, so if we keep the parsed JSON object we can just set to that > directly. Done. https://codereview.appspot.com/9618044/diff/14001/google-http-client/src/main... File google-http-client/src/main/java/com/google/api/client/json/Polymorphic.java (right): https://codereview.appspot.com/9618044/diff/14001/google-http-client/src/main... google-http-client/src/main/java/com/google/api/client/json/Polymorphic.java:28: public @interface Polymorphic { On 2013/06/04 20:51:03, yanivi wrote: > Do we want this annotation? The alternative is to check every loaded class for > a field with JsonTypeMap annotation. It could be as simple to implement as > using null Field value in cachedTypemapFields to indicate that we looked for > such a field but didn't find it. So I mostly included this annotation in the code review because I was going back and forth with the same debate. Even if we do cache the "JsonTypeMap not found" value, the first time we parse still has to go through every field in the class. So its a debate between a performance drop on the first parse, which for everyone except 1% of users will probably give no benefit, or require an extra bit of boiler-plate for those who are using it. I'm not sure which way to go. Preference? https://codereview.appspot.com/9618044/diff/14001/google-http-client/src/main... File google-http-client/src/main/java/com/google/api/client/json/TypeDef.java (right): https://codereview.appspot.com/9618044/diff/14001/google-http-client/src/main... google-http-client/src/main/java/com/google/api/client/json/TypeDef.java:28: public @interface TypeDef { On 2013/06/04 20:51:03, yanivi wrote: > should we move @interface TypedDef inside JsonTypeMap.java like Jackson does > with JsonSubTypes? Yes, I like that. It's rather clean. Done.
Sign in to reply to this message.
https://codereview.appspot.com/9618044/diff/14001/google-http-client/src/main... File google-http-client/src/main/java/com/google/api/client/json/JsonParser.java (right): https://codereview.appspot.com/9618044/diff/14001/google-http-client/src/main... google-http-client/src/main/java/com/google/api/client/json/JsonParser.java:804: if (!cachedTypemapFields.containsKey(newInstanceClass)) { On 2013/06/10 17:16:48, ngmiceli wrote: > On 2013/06/04 20:51:03, yanivi wrote: > > use locking to ensure thread-safety > > Done. Eyal suggested I use double-checked locking here > (http://www.ibm.com/developerworks/java/library/j-dcl/index.html) > Still being a little fresh at concurrency in practice, I'd like to hear your > opinion. Did you notice the subtitle? "A comprehensive look at this broken programming idiom". The Editor's note says that it is broken even under Java 5's memory model. Oops :) https://codereview.appspot.com/9618044/diff/14001/google-http-client/src/main... File google-http-client/src/main/java/com/google/api/client/json/Polymorphic.java (right): https://codereview.appspot.com/9618044/diff/14001/google-http-client/src/main... google-http-client/src/main/java/com/google/api/client/json/Polymorphic.java:28: public @interface Polymorphic { On 2013/06/10 17:16:48, ngmiceli wrote: > On 2013/06/04 20:51:03, yanivi wrote: > > Do we want this annotation? The alternative is to check every loaded class > for > > a field with JsonTypeMap annotation. It could be as simple to implement as > > using null Field value in cachedTypemapFields to indicate that we looked for > > such a field but didn't find it. > > So I mostly included this annotation in the code review because I was going back > and forth with the same debate. Even if we do cache the "JsonTypeMap not found" > value, the first time we parse still has to go through every field in the class. > So its a debate between a performance drop on the first parse, which for > everyone except 1% of users will probably give no benefit, or require an extra > bit of boiler-plate for those who are using it. I'm not sure which way to go. > Preference? Let's discuss this in person. https://codereview.appspot.com/9618044/diff/23001/google-http-client/src/main... File google-http-client/src/main/java/com/google/api/client/json/JsonParser.java (right): https://codereview.appspot.com/9618044/diff/23001/google-http-client/src/main... google-http-client/src/main/java/com/google/api/client/json/JsonParser.java:55: /** Keeps track of which field in a {@code @Polymorphic} datatype determines the type.**/ This comment is too confusing. What is the key? What is the value? I get it now, but it took me about 5 readings. https://codereview.appspot.com/9618044/diff/23001/google-http-client/src/main... google-http-client/src/main/java/com/google/api/client/json/JsonParser.java:56: private static Map<Class<?>, Field> cachedTypemapFields = new HashMap<Class<?>, Field>(); WeakHashMap so it releases memory when the system is low on memory. https://codereview.appspot.com/9618044/diff/23001/google-http-client/src/main... google-http-client/src/main/java/com/google/api/client/json/JsonParser.java:58: private final Lock lock = new ReentrantLock(); javadoc comment https://codereview.appspot.com/9618044/diff/23001/google-http-client/src/main... google-http-client/src/main/java/com/google/api/client/json/JsonParser.java:382: new ArrayList<UnusedJson>()); why isn't this null? we're only interested in the unused json in the case where we have a polymorphic type. similarly for most of the other callers that use the unusedjson parameter https://codereview.appspot.com/9618044/diff/23001/google-http-client/src/main... google-http-client/src/main/java/com/google/api/client/json/JsonParser.java:487: customizeParser.handleUnrecognizedKey(context, key); Don't change this line. We don't want any incompatible changes to CustomizeJsonParser behavior. https://codereview.appspot.com/9618044/diff/23001/google-http-client/src/main... google-http-client/src/main/java/com/google/api/client/json/JsonParser.java:489: if (unusedJsonList != null) { reduce code duplication by merging this with the previous section. also, skipChildren should only be done if we don't parse. so it'll be something roughly like: GenericData object = isGenericData ? (GenericData) destination : unusedJsonList != null ? new GenericJson() : null; if (object != null) { Object parsed = parseValue(...); object.set(key, parsed); } else { skipChildren(); } https://codereview.appspot.com/9618044/diff/23001/google-http-client/src/main... google-http-client/src/main/java/com/google/api/client/json/JsonParser.java:715: * @param customizeParser customize parser or {@code null} for none missing JavaDoc @param unusedJsonList https://codereview.appspot.com/9618044/diff/23001/google-http-client/src/main... google-http-client/src/main/java/com/google/api/client/json/JsonParser.java:723: ArrayList<UnusedJson> unusedJsonList) throws IOException { perhaps "Generic unusedJson" might be a simpler choice? we anyway allocate a new instance of GenericJson, so we might as well just make unusedJson be that object. we might as well serialize to string later on, and actually we may be able to take some short-cuts, like you don't need to do any serialize/deserialize if the field type is primitive like strings and numbers. https://codereview.appspot.com/9618044/diff/23001/google-http-client/src/main... google-http-client/src/main/java/com/google/api/client/json/JsonParser.java:806: parse(context, newInstance, customizeParser, unusedJsonList); here we should be allocating a new instance of unusedJson. It is the only scenario where we actually need it. https://codereview.appspot.com/9618044/diff/23001/google-http-client/src/main... google-http-client/src/main/java/com/google/api/client/json/JsonParser.java:806: parse(context, newInstance, customizeParser, unusedJsonList); TODO to think about whether we can optimize for case for example where the type is at the beginning or middle so we can parse to the correct type from that point https://codereview.appspot.com/9618044/diff/23001/google-http-client/src/main... google-http-client/src/main/java/com/google/api/client/json/JsonParser.java:835: for (UnusedJson unusedJson : unusedJsonList) { do we need a null check on unusedJsonList? https://codereview.appspot.com/9618044/diff/23001/google-http-client/src/main... google-http-client/src/main/java/com/google/api/client/json/JsonParser.java:839: getFactory().createJsonParser(unusedJson.json).parse(typeClass); TODO to optimize for not doing serialization/deserialization and doing a conversion from genericjson to specific object. https://codereview.appspot.com/9618044/diff/23001/google-http-client/src/main... google-http-client/src/main/java/com/google/api/client/json/JsonParser.java:851: } catch (ReflectiveOperationException e) { Description Resource Path Location Type ReflectiveOperationException cannot be resolved to a type JsonParser.java /google-http-client/src/main/java/com/google/api/client/json line 851 Java Problem note the @since 1.7 on ReflectiveOperationException https://codereview.appspot.com/9618044/diff/23001/google-http-client/src/main... google-http-client/src/main/java/com/google/api/client/json/JsonParser.java:933: private Field getCachedTypemapFieldFor(Class<?> key) { missing JavaDoc. https://codereview.appspot.com/9618044/diff/23001/google-http-client/src/main... google-http-client/src/main/java/com/google/api/client/json/JsonParser.java:934: if (!cachedTypemapFields.containsKey(key)) { Remove this line. This must happen inside the lock. Otherwise, you cannot establish a happens-before relationship from the time it was written and the time it was read. In other words, here you may be reading the stable old value before it was written to, even though the write has already happened in another thread. https://codereview.appspot.com/9618044/diff/23001/google-http-client/src/main... google-http-client/src/main/java/com/google/api/client/json/JsonParser.java:940: if (field.isAnnotationPresent(JsonTypeMap.class)) { should we throw an IllegalArgumentException if multiple fields have the JsonTypeMap annotation? https://codereview.appspot.com/9618044/diff/23001/google-http-client/src/main... google-http-client/src/main/java/com/google/api/client/json/JsonParser.java:949: return null; perhaps better to throw an IllegalArgumentException? https://codereview.appspot.com/9618044/diff/23001/google-http-client/src/main... google-http-client/src/main/java/com/google/api/client/json/JsonParser.java:951: return cachedTypemapFields.get(key); remove this line for the same reason as above.
Sign in to reply to this message.
Since you've started reviewing small details and javadoc, I guess we should turn this from [Work in Progress] to full review mode. I am aware however that this changeset is still lacking some tests. Let's make some time to sit down and come up with ideas for more convoluted scenarios to ensure I've covered all the odd corner-cases. https://codereview.appspot.com/9618044/diff/23001/google-http-client/src/main... File google-http-client/src/main/java/com/google/api/client/json/JsonParser.java (right): https://codereview.appspot.com/9618044/diff/23001/google-http-client/src/main... google-http-client/src/main/java/com/google/api/client/json/JsonParser.java:55: /** Keeps track of which field in a {@code @Polymorphic} datatype determines the type.**/ On 2013/06/13 11:29:49, yanivi wrote: > This comment is too confusing. What is the key? What is the value? I get it > now, but it took me about 5 readings. Better? https://codereview.appspot.com/9618044/diff/23001/google-http-client/src/main... google-http-client/src/main/java/com/google/api/client/json/JsonParser.java:56: private static Map<Class<?>, Field> cachedTypemapFields = new HashMap<Class<?>, Field>(); On 2013/06/13 11:29:49, yanivi wrote: > WeakHashMap so it releases memory when the system is low on memory. Done. https://codereview.appspot.com/9618044/diff/23001/google-http-client/src/main... google-http-client/src/main/java/com/google/api/client/json/JsonParser.java:58: private final Lock lock = new ReentrantLock(); On 2013/06/13 11:29:49, yanivi wrote: > javadoc comment Done. https://codereview.appspot.com/9618044/diff/23001/google-http-client/src/main... google-http-client/src/main/java/com/google/api/client/json/JsonParser.java:382: new ArrayList<UnusedJson>()); On 2013/06/13 11:29:49, yanivi wrote: > why isn't this null? we're only interested in the unused json in the case where > we have a polymorphic type. > > similarly for most of the other callers that use the unusedjson parameter Any of the entrance points into parse could be dealing with a Polymorphic type. I added the check in other methods to it to make it smarter, but I can't here because AFAIK Type does not know about Annotations (dataType.getClass.isAnnotationPresent always returns false) https://codereview.appspot.com/9618044/diff/23001/google-http-client/src/main... google-http-client/src/main/java/com/google/api/client/json/JsonParser.java:487: customizeParser.handleUnrecognizedKey(context, key); On 2013/06/13 11:29:49, yanivi wrote: > Don't change this line. We don't want any incompatible changes to > CustomizeJsonParser behavior. Oversight - done. https://codereview.appspot.com/9618044/diff/23001/google-http-client/src/main... google-http-client/src/main/java/com/google/api/client/json/JsonParser.java:489: if (unusedJsonList != null) { On 2013/06/13 11:29:49, yanivi wrote: > reduce code duplication by merging this with the previous section. also, > skipChildren should only be done if we don't parse. so it'll be something > roughly like: > > GenericData object = isGenericData ? (GenericData) destination : > unusedJsonList != null ? new GenericJson() : null; > if (object != null) { > Object parsed = parseValue(...); > object.set(key, parsed); > } else { > skipChildren(); > } Done. Unfortunately, readability takes a hit in exchange for brevity. https://codereview.appspot.com/9618044/diff/23001/google-http-client/src/main... google-http-client/src/main/java/com/google/api/client/json/JsonParser.java:715: * @param customizeParser customize parser or {@code null} for none On 2013/06/13 11:29:49, yanivi wrote: > missing JavaDoc @param unusedJsonList Done. https://codereview.appspot.com/9618044/diff/23001/google-http-client/src/main... google-http-client/src/main/java/com/google/api/client/json/JsonParser.java:723: ArrayList<UnusedJson> unusedJsonList) throws IOException { On 2013/06/13 11:29:49, yanivi wrote: > perhaps "Generic unusedJson" might be a simpler choice? I'm not sure what you mean here > we anyway allocate a new instance of GenericJson, so we might as well just make > unusedJson be that object. we might as well serialize to string later on, and > actually we may be able to take some short-cuts, like you don't need to do any > serialize/deserialize if the field type is primitive like strings and numbers. Replaced the String field "json" with GenericData. Not sure if that's what you meant, but it does seem to allow for some shortcuts. https://codereview.appspot.com/9618044/diff/23001/google-http-client/src/main... google-http-client/src/main/java/com/google/api/client/json/JsonParser.java:806: parse(context, newInstance, customizeParser, unusedJsonList); On 2013/06/13 11:29:49, yanivi wrote: > here we should be allocating a new instance of unusedJson. It is the only > scenario where we actually need it. I'm not sure if I agree/understand -- let's talk about this in person. https://codereview.appspot.com/9618044/diff/23001/google-http-client/src/main... google-http-client/src/main/java/com/google/api/client/json/JsonParser.java:806: parse(context, newInstance, customizeParser, unusedJsonList); On 2013/06/13 11:29:49, yanivi wrote: > TODO to think about whether we can optimize for case for example where the type > is at the beginning or middle so we can parse to the correct type from that > point Done. https://codereview.appspot.com/9618044/diff/23001/google-http-client/src/main... google-http-client/src/main/java/com/google/api/client/json/JsonParser.java:835: for (UnusedJson unusedJson : unusedJsonList) { On 2013/06/13 11:29:49, yanivi wrote: > do we need a null check on unusedJsonList? I don't think so. If so, then I think my mistake is not initializing the list somewhere I should https://codereview.appspot.com/9618044/diff/23001/google-http-client/src/main... google-http-client/src/main/java/com/google/api/client/json/JsonParser.java:839: getFactory().createJsonParser(unusedJson.json).parse(typeClass); On 2013/06/13 11:29:49, yanivi wrote: > TODO to optimize for not doing serialization/deserialization and doing a > conversion from genericjson to specific object. Done. https://codereview.appspot.com/9618044/diff/23001/google-http-client/src/main... google-http-client/src/main/java/com/google/api/client/json/JsonParser.java:851: } catch (ReflectiveOperationException e) { On 2013/06/13 11:29:49, yanivi wrote: > Description Resource Path Location Type > ReflectiveOperationException cannot be resolved to a > type JsonParser.java /google-http-client/src/main/java/com/google/api/client/json line > 851 Java Problem > > note the @since 1.7 on ReflectiveOperationException Done. https://codereview.appspot.com/9618044/diff/23001/google-http-client/src/main... google-http-client/src/main/java/com/google/api/client/json/JsonParser.java:933: private Field getCachedTypemapFieldFor(Class<?> key) { On 2013/06/13 11:29:49, yanivi wrote: > missing JavaDoc. Done. https://codereview.appspot.com/9618044/diff/23001/google-http-client/src/main... google-http-client/src/main/java/com/google/api/client/json/JsonParser.java:934: if (!cachedTypemapFields.containsKey(key)) { On 2013/06/13 11:29:49, yanivi wrote: > Remove this line. This must happen inside the lock. Otherwise, you cannot > establish a happens-before relationship from the time it was written and the > time it was read. In other words, here you may be reading the stable old value > before it was written to, even though the write has already happened in another > thread. Done. https://codereview.appspot.com/9618044/diff/23001/google-http-client/src/main... google-http-client/src/main/java/com/google/api/client/json/JsonParser.java:940: if (field.isAnnotationPresent(JsonTypeMap.class)) { On 2013/06/13 11:29:49, yanivi wrote: > should we throw an IllegalArgumentException if multiple fields have the > JsonTypeMap annotation? Hmm. Interesting suggestion, but that means extra work for the method (it must always check all Fields instead of stopping at the first Field). I say no: we specify in the Javadoc for @Polymorphic that it must contain exactly one @JsonTypeMap field. If they break that contract, it will simply use the first matching Field. Disagree? https://codereview.appspot.com/9618044/diff/23001/google-http-client/src/main... google-http-client/src/main/java/com/google/api/client/json/JsonParser.java:949: return null; On 2013/06/13 11:29:49, yanivi wrote: > perhaps better to throw an IllegalArgumentException? Depends on whether or not we choose to keep @Polymorphic. If we do, then yes it is an exception if the class has @Polymorphic but not @JsonTypeMap. If not, then no as null is a useful answer (sorry, this class does not have that annotation). For now I'll go with the exception since we're using @Polymorphic. https://codereview.appspot.com/9618044/diff/23001/google-http-client/src/main... google-http-client/src/main/java/com/google/api/client/json/JsonParser.java:951: return cachedTypemapFields.get(key); On 2013/06/13 11:29:49, yanivi wrote: > remove this line for the same reason as above. Done.
Sign in to reply to this message.
I started writing some feedback, but I realized quickly that we're not going to get anywhere like this. Let's meet in person to review the code together... https://codereview.appspot.com/9618044/diff/23001/google-http-client/src/main... File google-http-client/src/main/java/com/google/api/client/json/JsonParser.java (right): https://codereview.appspot.com/9618044/diff/23001/google-http-client/src/main... google-http-client/src/main/java/com/google/api/client/json/JsonParser.java:475: // store unknown field in generic JSON why remove this comment? https://codereview.appspot.com/9618044/diff/23001/google-http-client/src/main... google-http-client/src/main/java/com/google/api/client/json/JsonParser.java:485: // unrecognized field, skip value why remove this comment? https://codereview.appspot.com/9618044/diff/23001/google-http-client/src/main... google-http-client/src/main/java/com/google/api/client/json/JsonParser.java:489: if (unusedJsonList != null) { On 2013/06/14 22:04:46, ngmiceli wrote: > On 2013/06/13 11:29:49, yanivi wrote: > > reduce code duplication by merging this with the previous section. also, > > skipChildren should only be done if we don't parse. so it'll be something > > roughly like: > > > > GenericData object = isGenericData ? (GenericData) destination : > > unusedJsonList != null ? new GenericJson() : null; > > if (object != null) { > > Object parsed = parseValue(...); > > object.set(key, parsed); > > } else { > > skipChildren(); > > } > > Done. Unfortunately, readability takes a hit in exchange for brevity. Brevity wasn't my goal, just reduced code duplication. You are welcome and encouraged to write better comments and split off "?:" statement into real if-statements when it helps readability. I was trying to convey the idea, not write your code for you. https://codereview.appspot.com/9618044/diff/23001/google-http-client/src/main... google-http-client/src/main/java/com/google/api/client/json/JsonParser.java:723: ArrayList<UnusedJson> unusedJsonList) throws IOException { On 2013/06/14 22:04:46, ngmiceli wrote: > On 2013/06/13 11:29:49, yanivi wrote: > > perhaps "Generic unusedJson" might be a simpler choice? > I'm not sure what you mean here I literally mean "Generic unusedJson" here instead of "ArrayList<UnusedJson> unusedJsonList". > > we anyway allocate a new instance of GenericJson, so we might as well just > make > > unusedJson be that object. we might as well serialize to string later on, > and > > actually we may be able to take some short-cuts, like you don't need to do any > > serialize/deserialize if the field type is primitive like strings and numbers. > Replaced the String field "json" with GenericData. Not sure if that's what you > meant, but it does seem to allow for some shortcuts. https://codereview.appspot.com/9618044/diff/40001/google-http-client/src/main... File google-http-client/src/main/java/com/google/api/client/json/JsonParser.java (right): https://codereview.appspot.com/9618044/diff/40001/google-http-client/src/main... google-http-client/src/main/java/com/google/api/client/json/JsonParser.java:435: private void parse(ArrayList<Type> context, Object destination, Please add a JavaDoc comment. Sorry, my fault for not writing it in the first place.
Sign in to reply to this message.
https://codereview.appspot.com/9618044/diff/40001/google-http-client/src/main... File google-http-client/src/main/java/com/google/api/client/json/JsonParser.java (right): https://codereview.appspot.com/9618044/diff/40001/google-http-client/src/main... google-http-client/src/main/java/com/google/api/client/json/JsonParser.java:966: cachedTypemapFields.put(key, field); throw IllegalARgumentException if Field doesn't have type String?
Sign in to reply to this message.
please add a link to https://code.google.com/p/google-http-java-client/issues/detail?id=173 in your changeset description
Sign in to reply to this message.
Yaniv and I discussed the next round of comments in person: 1) Add a test using arrays Done. 2) Remove @Polymorphic. Check if a class is polymorphic the first time (by having exactly one typemap) and cache if it isn't (by storing null in the typemap cache) Done. 3) Use GenericJson instead of ArrayList<UnusedJson> Done. 4) Add a test where Animal extends GenericJson Already had this. 5) The IOException constructor(String, Throwable) is @since 1.6, we support 1.5. Fixed. 6) Throw an exception when checking if the class is Polymorphic if there are multiple typemap annotations. Done. 7) Use ClassInfo.getNames instead of reflection. Reflection will only grab public fields, so test this change on non-public fields. Done.
Sign in to reply to this message.
https://codereview.appspot.com/9618044/diff/52001/google-http-client-test/src... File google-http-client-test/src/main/java/com/google/api/client/test/json/AbstractJsonFactoryTest.java (right): https://codereview.appspot.com/9618044/diff/52001/google-http-client-test/src... google-http-client-test/src/main/java/com/google/api/client/test/json/AbstractJsonFactoryTest.java:1446: public static class Animal { ctrl-shift-f this file https://codereview.appspot.com/9618044/diff/52001/google-http-client-test/src... google-http-client-test/src/main/java/com/google/api/client/test/json/AbstractJsonFactoryTest.java:1446: public static class Animal { add a test where type property is a number https://codereview.appspot.com/9618044/diff/52001/google-http-client/src/main... File google-http-client/src/main/java/com/google/api/client/json/JsonParser.java (right): https://codereview.appspot.com/9618044/diff/52001/google-http-client/src/main... google-http-client/src/main/java/com/google/api/client/json/JsonParser.java:58: * Maps a Polymorphic {@code Class} its {@code Field} with {@code @JsonTypeMap}, as that field's I see a bunch of problems with this comment. To summarize, I'd like something like: "Maps a polymorphic {@link Class} to it {@link Field} with {@link JsonPolymorphicTypeMap} or {@code null} if there is no field with that annotation." https://codereview.appspot.com/9618044/diff/52001/google-http-client/src/main... google-http-client/src/main/java/com/google/api/client/json/JsonParser.java:420: private void parse(ArrayList<Type> context, Object destination, missing JavaDoc comment https://codereview.appspot.com/9618044/diff/52001/google-http-client/src/main... google-http-client/src/main/java/com/google/api/client/json/JsonParser.java:470: unusedJson.set(key, parsed); if (unusedJson != null) { ...} else if (isGenericData) { ... } otherwise, if both are true then your logic below would break https://codereview.appspot.com/9618044/diff/52001/google-http-client/src/main... google-http-client/src/main/java/com/google/api/client/json/JsonParser.java:756: Class<?> newInstanceClass = newInstance.getClass(); instead of newInstanceClass use valueClass https://codereview.appspot.com/9618044/diff/52001/google-http-client/src/main... google-http-client/src/main/java/com/google/api/client/json/JsonParser.java:773: String typeValue = (String) typemapField.get(newInstance); instead of cast to "(String)" do .toString() to handle other primitives https://codereview.appspot.com/9618044/diff/52001/google-http-client/src/main... google-http-client/src/main/java/com/google/api/client/json/JsonParser.java:785: if (unusedJson.size() > 0) { if (!unusedJson.isEmpty()) https://codereview.appspot.com/9618044/diff/52001/google-http-client/src/main... google-http-client/src/main/java/com/google/api/client/json/JsonParser.java:790: for (Entry<String, Object> unused : unusedJson.entrySet()) { rename unused to unusedEntry https://codereview.appspot.com/9618044/diff/52001/google-http-client/src/main... google-http-client/src/main/java/com/google/api/client/json/JsonParser.java:791: Field destinationField = typeClassInfo.getField(unused.getKey()); String name = unusedEntry.getKey() and reuse it in both places. https://codereview.appspot.com/9618044/diff/52001/google-http-client/src/main... google-http-client/src/main/java/com/google/api/client/json/JsonParser.java:907: if (cachedTypemapFields.containsKey(key)) { this slightly more compact, more readable, and more efficient: Field value = cachedTypemapFields.get(key); if (value != null) { return value; } https://codereview.appspot.com/9618044/diff/52001/google-http-client/src/main... google-http-client/src/main/java/com/google/api/client/json/JsonParser.java:915: value = field; Preconditions.checkArgument(Data.isPrimitive(field)); https://codereview.appspot.com/9618044/diff/52001/google-http-client/src/main... google-http-client/src/main/java/com/google/api/client/json/JsonParser.java:917: throw new IllegalArgumentException( Preconditions.checkArgument(value == null, "Class contains more than one field with @JsonPolymorphicTypeMap annotation: %s", key); https://codereview.appspot.com/9618044/diff/52001/google-http-client/src/main... google-http-client/src/main/java/com/google/api/client/json/JsonParser.java:935: private List<Field> getFieldsFor(Class<?> srcClass) { better to replace this method with something roughly like this in ClassInfo: /** * Returns an unmodifiable collection of the field infos without any guarantee of order. * * <p> * If you need sorted order, instead use {@link #getNames()} with {@link #getFieldInfo(String)}. * </p> * * @since 1.16 */ public Collection<FieldInfo> getFieldInfos() { return Collections.unmodifiableCollection(nameToFieldInfoMap.values()); } https://codereview.appspot.com/9618044/diff/52001/google-http-client/src/main... File google-http-client/src/main/java/com/google/api/client/json/JsonPolymorphicTypeMap.java (right): https://codereview.appspot.com/9618044/diff/52001/google-http-client/src/main... google-http-client/src/main/java/com/google/api/client/json/JsonPolymorphicTypeMap.java:36: public @interface JsonPolymorphicTypeMap { add @Beta to this annotation
Sign in to reply to this message.
https://codereview.appspot.com/9618044/diff/52001/google-http-client-test/src... File google-http-client-test/src/main/java/com/google/api/client/test/json/AbstractJsonFactoryTest.java (right): https://codereview.appspot.com/9618044/diff/52001/google-http-client-test/src... google-http-client-test/src/main/java/com/google/api/client/test/json/AbstractJsonFactoryTest.java:1446: public static class Animal { On 2013/06/24 15:41:57, yanivi wrote: > ctrl-shift-f this file Done. https://codereview.appspot.com/9618044/diff/52001/google-http-client-test/src... google-http-client-test/src/main/java/com/google/api/client/test/json/AbstractJsonFactoryTest.java:1446: public static class Animal { On 2013/06/24 15:41:57, yanivi wrote: > add a test where type property is a number Done. https://codereview.appspot.com/9618044/diff/52001/google-http-client/src/main... File google-http-client/src/main/java/com/google/api/client/json/JsonParser.java (right): https://codereview.appspot.com/9618044/diff/52001/google-http-client/src/main... google-http-client/src/main/java/com/google/api/client/json/JsonParser.java:58: * Maps a Polymorphic {@code Class} its {@code Field} with {@code @JsonTypeMap}, as that field's On 2013/06/24 15:41:57, yanivi wrote: > I see a bunch of problems with this comment. To summarize, I'd like something > like: > > "Maps a polymorphic {@link Class} to it {@link Field} with {@link > JsonPolymorphicTypeMap} or {@code null} if there is no field with that > annotation." Done. https://codereview.appspot.com/9618044/diff/52001/google-http-client/src/main... google-http-client/src/main/java/com/google/api/client/json/JsonParser.java:420: private void parse(ArrayList<Type> context, Object destination, On 2013/06/24 15:41:57, yanivi wrote: > missing JavaDoc comment Done. https://codereview.appspot.com/9618044/diff/52001/google-http-client/src/main... google-http-client/src/main/java/com/google/api/client/json/JsonParser.java:470: unusedJson.set(key, parsed); On 2013/06/24 15:41:57, yanivi wrote: > if (unusedJson != null) { ...} else if (isGenericData) { ... } > > otherwise, if both are true then your logic below would break Perhaps I'm misunderstanding your comment but I don't see how my logic could break here. In addition, I have a test where unusedJson is not null and the object extends GenericJson (testParser_heterogeneousSchema_genericJson()) EDIT: Doesn't matter, I removed unusedJson. https://codereview.appspot.com/9618044/diff/52001/google-http-client/src/main... google-http-client/src/main/java/com/google/api/client/json/JsonParser.java:756: Class<?> newInstanceClass = newInstance.getClass(); On 2013/06/24 15:41:57, yanivi wrote: > instead of newInstanceClass use valueClass Done. https://codereview.appspot.com/9618044/diff/52001/google-http-client/src/main... google-http-client/src/main/java/com/google/api/client/json/JsonParser.java:773: String typeValue = (String) typemapField.get(newInstance); On 2013/06/24 15:41:57, yanivi wrote: > instead of cast to "(String)" do .toString() to handle other primitives Done. https://codereview.appspot.com/9618044/diff/52001/google-http-client/src/main... google-http-client/src/main/java/com/google/api/client/json/JsonParser.java:785: if (unusedJson.size() > 0) { On 2013/06/24 15:41:57, yanivi wrote: > if (!unusedJson.isEmpty()) Done. https://codereview.appspot.com/9618044/diff/52001/google-http-client/src/main... google-http-client/src/main/java/com/google/api/client/json/JsonParser.java:790: for (Entry<String, Object> unused : unusedJson.entrySet()) { On 2013/06/24 15:41:57, yanivi wrote: > rename unused to unusedEntry Done. https://codereview.appspot.com/9618044/diff/52001/google-http-client/src/main... google-http-client/src/main/java/com/google/api/client/json/JsonParser.java:791: Field destinationField = typeClassInfo.getField(unused.getKey()); On 2013/06/24 15:41:57, yanivi wrote: > String name = unusedEntry.getKey() and reuse it in both places. Done. https://codereview.appspot.com/9618044/diff/52001/google-http-client/src/main... google-http-client/src/main/java/com/google/api/client/json/JsonParser.java:907: if (cachedTypemapFields.containsKey(key)) { On 2013/06/24 15:41:57, yanivi wrote: > this slightly more compact, more readable, and more efficient: > > Field value = cachedTypemapFields.get(key); > if (value != null) { > return value; > } No can do. A value of null has meaning - it means the class has been investigated and does not contain the field. Using it like this would mean we can't distinguish between having not been investigated yet or not having the field. If the cache DOES contain a key, and its value is null, your code would incorrectly run through the check-logic again instead of returning null. https://codereview.appspot.com/9618044/diff/52001/google-http-client/src/main... google-http-client/src/main/java/com/google/api/client/json/JsonParser.java:915: value = field; On 2013/06/24 15:41:57, yanivi wrote: > Preconditions.checkArgument(Data.isPrimitive(field)); Done. https://codereview.appspot.com/9618044/diff/52001/google-http-client/src/main... google-http-client/src/main/java/com/google/api/client/json/JsonParser.java:917: throw new IllegalArgumentException( On 2013/06/24 15:41:57, yanivi wrote: > Preconditions.checkArgument(value == null, "Class contains more than one field > with @JsonPolymorphicTypeMap annotation: %s", key); Done. https://codereview.appspot.com/9618044/diff/52001/google-http-client/src/main... google-http-client/src/main/java/com/google/api/client/json/JsonParser.java:935: private List<Field> getFieldsFor(Class<?> srcClass) { On 2013/06/24 15:41:57, yanivi wrote: > better to replace this method with something roughly like this in ClassInfo: > > /** > * Returns an unmodifiable collection of the field infos without any guarantee > of order. > * > * <p> > * If you need sorted order, instead use {@link #getNames()} with {@link > #getFieldInfo(String)}. > * </p> > * > * @since 1.16 > */ > public Collection<FieldInfo> getFieldInfos() { > return Collections.unmodifiableCollection(nameToFieldInfoMap.values()); > } Done. https://codereview.appspot.com/9618044/diff/52001/google-http-client/src/main... File google-http-client/src/main/java/com/google/api/client/json/JsonPolymorphicTypeMap.java (right): https://codereview.appspot.com/9618044/diff/52001/google-http-client/src/main... google-http-client/src/main/java/com/google/api/client/json/JsonPolymorphicTypeMap.java:36: public @interface JsonPolymorphicTypeMap { On 2013/06/24 15:41:57, yanivi wrote: > add @Beta to this annotation Done.
Sign in to reply to this message.
hopefully this is the last round of comments... https://codereview.appspot.com/9618044/diff/75001/google-http-client-test/src... File google-http-client-test/src/main/java/com/google/api/client/test/json/AbstractJsonFactoryTest.java (right): https://codereview.appspot.com/9618044/diff/75001/google-http-client-test/src... google-http-client-test/src/main/java/com/google/api/client/test/json/AbstractJsonFactoryTest.java:1446: public static abstract class Animal { /Users/yanivi/hg/google-http-java-client/default/default/google-http-client-test/src/main/java/com/google/api/client/test/json/AbstractJsonFactoryTest.java:1446:17: 'abstract' modifier out of order with the JLS suggestions. https://codereview.appspot.com/9618044/diff/75001/google-http-client/src/main... File google-http-client/src/main/java/com/google/api/client/json/JsonParser.java (right): https://codereview.appspot.com/9618044/diff/75001/google-http-client/src/main... google-http-client/src/main/java/com/google/api/client/json/JsonParser.java:64: private final Lock lock = new ReentrantLock(); static https://codereview.appspot.com/9618044/diff/75001/google-http-client/src/main... google-http-client/src/main/java/com/google/api/client/json/JsonParser.java:470: if (!isGenericData && customizeParser != null) { please revert this code back to its original state in the Base changeset now that the logic is no longer different from the original that would also fix some of the style and one functional bug here, as well as reducing the diff https://codereview.appspot.com/9618044/diff/75001/google-http-client/src/main... google-http-client/src/main/java/com/google/api/client/json/JsonParser.java:696: boolean handlePolymorphic) throws IOException { missing JavaDoc for handlePolymorphic https://codereview.appspot.com/9618044/diff/75001/google-http-client/src/main... google-http-client/src/main/java/com/google/api/client/json/JsonParser.java:744: Field typemapField = getCachedTypemapFieldFor(valueClass); [optional] I might try: Field typemapField = handlePolymorphic ? getCachedTypemapFieldFor(valueClass) : null; and then skip the isPolymorphic boolean just to reduce the number of local variables. The more variables there are the harder it gets to keep track of all of them and what each means. Like is it possible to have isPolymorphic but typemapField == null? Obviously not, but harder to remember that if you're 50 lines further in the source file. https://codereview.appspot.com/9618044/diff/75001/google-http-client/src/main... google-http-client/src/main/java/com/google/api/client/json/JsonParser.java:775: [optional] I'd prefer not to randomly insert extra whitespace in the code, especially when in an area that you are not editing https://codereview.appspot.com/9618044/diff/75001/google-http-client/src/main... google-http-client/src/main/java/com/google/api/client/json/JsonParser.java:787: // TODO(ngmiceli): Can we optimize this better? What if the type is first? Middle? I would combine this TODO with the TODO at line 804 https://codereview.appspot.com/9618044/diff/75001/google-http-client/src/main... google-http-client/src/main/java/com/google/api/client/json/JsonParser.java:788: Class<?> typeClass = null; style: move this to above line 794 so it is closer to where it is used https://codereview.appspot.com/9618044/diff/75001/google-http-client/src/main... google-http-client/src/main/java/com/google/api/client/json/JsonParser.java:792: typeValueObject != null, "No value specified for @JsonPolymorphicTypeMap field"); I think you should combine this message with the one below, basically saying that the type value is not one of the expected ones https://codereview.appspot.com/9618044/diff/75001/google-http-client/src/main... google-http-client/src/main/java/com/google/api/client/json/JsonParser.java:804: // TODO(ngmiceli): Avoid having to serialize unusedJson content twice. "serialize unusedJson" -> "parse JSON" https://codereview.appspot.com/9618044/diff/75001/google-http-client/src/main... google-http-client/src/main/java/com/google/api/client/json/JsonParser.java:807: return parser.parseValue(null, typeClass, new ArrayList<Type>(), null, null, false); 1st param: fieldContext (helps with error message) 3rd param: context (used to handle generic types T -> try adding a generic type to the polymorphic class) https://codereview.appspot.com/9618044/diff/75001/google-http-client/src/main... google-http-client/src/main/java/com/google/api/client/json/JsonParser.java:892: * @param key The {@link Class} to search in or null ... https://codereview.appspot.com/9618044/diff/75001/google-http-client/src/main... google-http-client/src/main/java/com/google/api/client/json/JsonParser.java:894: * if there is none or if key is null https://codereview.appspot.com/9618044/diff/75001/google-http-client/src/main... google-http-client/src/main/java/com/google/api/client/json/JsonParser.java:896: private Field getCachedTypemapFieldFor(Class<?> key) { static https://codereview.appspot.com/9618044/diff/75001/google-http-client/src/main... google-http-client/src/main/java/com/google/api/client/json/JsonParser.java:902: if (cachedTypemapFields.containsKey(key)) { comment to remind reader that must use containsKey because value null means it has already been computed https://codereview.appspot.com/9618044/diff/75001/google-http-client/src/main... google-http-client/src/main/java/com/google/api/client/json/JsonParser.java:908: HashSet<String> typeDefKeys = new HashSet<String>(); Sets.newHashSet() https://codereview.appspot.com/9618044/diff/75001/google-http-client/src/main... google-http-client/src/main/java/com/google/api/client/json/JsonParser.java:908: HashSet<String> typeDefKeys = new HashSet<String>(); move typeDefKeys below closer to where it is actually used https://codereview.appspot.com/9618044/diff/75001/google-http-client/src/main... google-http-client/src/main/java/com/google/api/client/json/JsonParser.java:920: TypeDef[] typeDefs = field.getAnnotation(JsonPolymorphicTypeMap.class).typeDefinitions(); above: JsonPolymorphicTypeMap annotation = field.getAnnotation(JsonPolymorphicTypeMap.class); if (annotation != null) { and then here: annotation.typeDefinitions() https://codereview.appspot.com/9618044/diff/75001/google-http-client/src/main... google-http-client/src/main/java/com/google/api/client/json/JsonParser.java:920: TypeDef[] typeDefs = field.getAnnotation(JsonPolymorphicTypeMap.class).typeDefinitions(); check that there is at least one typedef? https://codereview.appspot.com/9618044/diff/75001/google-http-client/src/main... File google-http-client/src/main/java/com/google/api/client/json/JsonPolymorphicTypeMap.java (right): https://codereview.appspot.com/9618044/diff/75001/google-http-client/src/main... google-http-client/src/main/java/com/google/api/client/json/JsonPolymorphicTypeMap.java:35: * type is not {@link String}, it will be compared against the {@link TypeDef#key()} using can remove the "not {@link String}" part because toString() covers that also https://codereview.appspot.com/9618044/diff/75001/google-http-client/src/main... google-http-client/src/main/java/com/google/api/client/json/JsonPolymorphicTypeMap.java:47: /** The list of mappings from key value to a referenced {@link Class}. */ [optional] just "class" instead of "{@link Class}"
Sign in to reply to this message.
https://codereview.appspot.com/9618044/diff/75001/google-http-client-test/src... File google-http-client-test/src/main/java/com/google/api/client/test/json/AbstractJsonFactoryTest.java (right): https://codereview.appspot.com/9618044/diff/75001/google-http-client-test/src... google-http-client-test/src/main/java/com/google/api/client/test/json/AbstractJsonFactoryTest.java:1446: public static abstract class Animal { On 2013/06/27 13:55:58, yanivi wrote: > /Users/yanivi/hg/google-http-java-client/default/default/google-http-client-test/src/main/java/com/google/api/client/test/json/AbstractJsonFactoryTest.java:1446:17: > 'abstract' modifier out of order with the JLS suggestions. Done. https://codereview.appspot.com/9618044/diff/75001/google-http-client/src/main... File google-http-client/src/main/java/com/google/api/client/json/JsonParser.java (right): https://codereview.appspot.com/9618044/diff/75001/google-http-client/src/main... google-http-client/src/main/java/com/google/api/client/json/JsonParser.java:64: private final Lock lock = new ReentrantLock(); On 2013/06/27 13:55:58, yanivi wrote: > static Done. https://codereview.appspot.com/9618044/diff/75001/google-http-client/src/main... google-http-client/src/main/java/com/google/api/client/json/JsonParser.java:470: if (!isGenericData && customizeParser != null) { On 2013/06/27 13:55:58, yanivi wrote: > please revert this code back to its original state in the Base changeset now > that the logic is no longer different from the original > > that would also fix some of the style and one functional bug here, as well as > reducing the diff Done. https://codereview.appspot.com/9618044/diff/75001/google-http-client/src/main... google-http-client/src/main/java/com/google/api/client/json/JsonParser.java:696: boolean handlePolymorphic) throws IOException { On 2013/06/27 13:55:58, yanivi wrote: > missing JavaDoc for handlePolymorphic No, it's there. Not sure why this patchset isn't showing it. Hopefully the next upload will show it? https://codereview.appspot.com/9618044/diff/75001/google-http-client/src/main... google-http-client/src/main/java/com/google/api/client/json/JsonParser.java:744: Field typemapField = getCachedTypemapFieldFor(valueClass); On 2013/06/27 13:55:58, yanivi wrote: > [optional] I might try: > > Field typemapField = handlePolymorphic ? > getCachedTypemapFieldFor(valueClass) : null; > > and then skip the isPolymorphic boolean just to reduce the number of local > variables. The more variables there are the harder it gets to keep track of all > of them and what each means. Like is it possible to have isPolymorphic but > typemapField == null? Obviously not, but harder to remember that if you're 50 > lines further in the source file. Done. https://codereview.appspot.com/9618044/diff/75001/google-http-client/src/main... google-http-client/src/main/java/com/google/api/client/json/JsonParser.java:775: On 2013/06/27 13:55:58, yanivi wrote: > [optional] I'd prefer not to randomly insert extra whitespace in the code, > especially when in an area that you are not editing Done. https://codereview.appspot.com/9618044/diff/75001/google-http-client/src/main... google-http-client/src/main/java/com/google/api/client/json/JsonParser.java:787: // TODO(ngmiceli): Can we optimize this better? What if the type is first? Middle? On 2013/06/27 13:55:58, yanivi wrote: > I would combine this TODO with the TODO at line 804 Done. https://codereview.appspot.com/9618044/diff/75001/google-http-client/src/main... google-http-client/src/main/java/com/google/api/client/json/JsonParser.java:788: Class<?> typeClass = null; On 2013/06/27 13:55:58, yanivi wrote: > style: move this to above line 794 so it is closer to where it is used Done. https://codereview.appspot.com/9618044/diff/75001/google-http-client/src/main... google-http-client/src/main/java/com/google/api/client/json/JsonParser.java:792: typeValueObject != null, "No value specified for @JsonPolymorphicTypeMap field"); On 2013/06/27 13:55:58, yanivi wrote: > I think you should combine this message with the one below, basically saying > that the type value is not one of the expected ones I need to ensure typeValueObject is not null before I use it right after this line. Same with typeClass. https://codereview.appspot.com/9618044/diff/75001/google-http-client/src/main... google-http-client/src/main/java/com/google/api/client/json/JsonParser.java:804: // TODO(ngmiceli): Avoid having to serialize unusedJson content twice. On 2013/06/27 13:55:58, yanivi wrote: > "serialize unusedJson" -> "parse JSON" Done. https://codereview.appspot.com/9618044/diff/75001/google-http-client/src/main... google-http-client/src/main/java/com/google/api/client/json/JsonParser.java:807: return parser.parseValue(null, typeClass, new ArrayList<Type>(), null, null, false); On 2013/06/27 13:55:58, yanivi wrote: > 1st param: fieldContext (helps with error message) > 3rd param: context (used to handle generic types T -> try adding a generic type > to the polymorphic class) Done. https://codereview.appspot.com/9618044/diff/75001/google-http-client/src/main... google-http-client/src/main/java/com/google/api/client/json/JsonParser.java:892: * @param key The {@link Class} to search in On 2013/06/27 13:55:58, yanivi wrote: > or null ... Done. https://codereview.appspot.com/9618044/diff/75001/google-http-client/src/main... google-http-client/src/main/java/com/google/api/client/json/JsonParser.java:894: * if there is none On 2013/06/27 13:55:58, yanivi wrote: > or if key is null Done. https://codereview.appspot.com/9618044/diff/75001/google-http-client/src/main... google-http-client/src/main/java/com/google/api/client/json/JsonParser.java:896: private Field getCachedTypemapFieldFor(Class<?> key) { On 2013/06/27 13:55:58, yanivi wrote: > static Done. https://codereview.appspot.com/9618044/diff/75001/google-http-client/src/main... google-http-client/src/main/java/com/google/api/client/json/JsonParser.java:902: if (cachedTypemapFields.containsKey(key)) { On 2013/06/27 13:55:58, yanivi wrote: > comment to remind reader that must use containsKey because value null means it > has already been computed Done. https://codereview.appspot.com/9618044/diff/75001/google-http-client/src/main... google-http-client/src/main/java/com/google/api/client/json/JsonParser.java:908: HashSet<String> typeDefKeys = new HashSet<String>(); On 2013/06/27 13:55:58, yanivi wrote: > move typeDefKeys below closer to where it is actually used Done. https://codereview.appspot.com/9618044/diff/75001/google-http-client/src/main... google-http-client/src/main/java/com/google/api/client/json/JsonParser.java:908: HashSet<String> typeDefKeys = new HashSet<String>(); On 2013/06/27 13:55:58, yanivi wrote: > Sets.newHashSet() Done... but I really don't see why this is necessary. https://codereview.appspot.com/9618044/diff/75001/google-http-client/src/main... google-http-client/src/main/java/com/google/api/client/json/JsonParser.java:920: TypeDef[] typeDefs = field.getAnnotation(JsonPolymorphicTypeMap.class).typeDefinitions(); On 2013/06/27 13:55:58, yanivi wrote: > above: > > JsonPolymorphicTypeMap annotation = > field.getAnnotation(JsonPolymorphicTypeMap.class); > if (annotation != null) { > > and then here: annotation.typeDefinitions() Done. https://codereview.appspot.com/9618044/diff/75001/google-http-client/src/main... google-http-client/src/main/java/com/google/api/client/json/JsonParser.java:920: TypeDef[] typeDefs = field.getAnnotation(JsonPolymorphicTypeMap.class).typeDefinitions(); On 2013/06/27 13:55:58, yanivi wrote: > check that there is at least one typedef? Done. https://codereview.appspot.com/9618044/diff/75001/google-http-client/src/main... File google-http-client/src/main/java/com/google/api/client/json/JsonPolymorphicTypeMap.java (right): https://codereview.appspot.com/9618044/diff/75001/google-http-client/src/main... google-http-client/src/main/java/com/google/api/client/json/JsonPolymorphicTypeMap.java:35: * type is not {@link String}, it will be compared against the {@link TypeDef#key()} using On 2013/06/27 13:55:58, yanivi wrote: > can remove the "not {@link String}" part because toString() covers that also Done. https://codereview.appspot.com/9618044/diff/75001/google-http-client/src/main... google-http-client/src/main/java/com/google/api/client/json/JsonPolymorphicTypeMap.java:47: /** The list of mappings from key value to a referenced {@link Class}. */ On 2013/06/27 13:55:58, yanivi wrote: > [optional] just "class" instead of "{@link Class}" Done.
Sign in to reply to this message.
LGTM https://codereview.appspot.com/9618044/diff/75001/google-http-client/src/main... File google-http-client/src/main/java/com/google/api/client/json/JsonParser.java (right): https://codereview.appspot.com/9618044/diff/75001/google-http-client/src/main... google-http-client/src/main/java/com/google/api/client/json/JsonParser.java:908: HashSet<String> typeDefKeys = new HashSet<String>(); On 2013/06/27 16:20:48, ngmiceli wrote: > On 2013/06/27 13:55:58, yanivi wrote: > > Sets.newHashSet() > > Done... but I really don't see why this is necessary. [fyi] for readability. Quote from our internal style guide: "local variables are declared close to the point they are first used (within reason), to minimize their scope"
Sign in to reply to this message.
On 2013/06/27 17:53:55, yanivi wrote: > LGTM > > https://codereview.appspot.com/9618044/diff/75001/google-http-client/src/main... > File google-http-client/src/main/java/com/google/api/client/json/JsonParser.java > (right): > > https://codereview.appspot.com/9618044/diff/75001/google-http-client/src/main... > google-http-client/src/main/java/com/google/api/client/json/JsonParser.java:908: > HashSet<String> typeDefKeys = new HashSet<String>(); > On 2013/06/27 16:20:48, ngmiceli wrote: > > On 2013/06/27 13:55:58, yanivi wrote: > > > Sets.newHashSet() > > > > Done... but I really don't see why this is necessary. > > [fyi] for readability. Quote from our internal style guide: "local variables > are declared close to the point they are first used (within reason), to minimize > their scope" No, you misread. That's definitely a good practice. It's using Sets.newHashSet (which just calls new HashSet<T>()) instead of calling new HashSet<>() myself. :)
Sign in to reply to this message.
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
