fix: correct ResourceOperationTypeAdapter dispatch and add guard

[sebthom]

• Replace reversed isAssignableFrom checks with instanceof to correctly handle subclasses (e.g., CreateFile derivatives) in write(...)
• Add explicit JsonParseException for unknown ResourceOperation subtypes in write(...) to mirror read(...) and avoid silent malformed JSON
• Improves protocol correctness and failure transparency when encountering unsupported operations

Tests: add ResourceOperationTypeAdapterTest covering subclass serialization and unknown subtype failure

[pisv]

@sebthom Good catch, thank you for the fix! 👍

It is already a great improvement, but would not it even better to check that the type passed to ResourceOperationTypeAdapter.create is assignable to CreateFile or DeleteFile or RenameFile? This would make it unnecessary to throw JsonParseException from InnerResourceOperationTypeAdapter.write.

(I don't quite like throwing JsonParseException from InnerResourceOperationTypeAdapter.write. According to its Javadoc, this exception is raised if there is a serious issue that occurs during parsing of a Json string. If anything, I'd prefer to throw AssertionError in this case.)

[sebthom]

Would that mean the ResourceOperationTypeAdapter.create should throw an error instead of returning null when the type does not match? And if so which Exception type? IllegalArgumentException, JsonParseException, JsonSyntaxException?

[pisv]

Would that mean the ResourceOperationTypeAdapter.create should throw an error instead of returning null when the type does not match?

As far as I can see, ResourceOperationTypeAdapter.create should just return null when the type does not match, just as it does currently. That is, I'd suggest to replace

		if (!ResourceOperation.class.isAssignableFrom(type.getRawType())) {
			return null;
		}

with

		if (!CreateFile.class.isAssignableFrom(type.getRawType()) &&
			!DeleteFile.class.isAssignableFrom(type.getRawType()) &&
			!RenameFile.class.isAssignableFrom(type.getRawType())) {
			return null;
		}

[sebthom]

This does not work type.getRawType() is always ResourceOperation.class and never one of the sub classes. I verified that via the debugger.

[pisv]

Thank you for verifying it. Looks like it was not a good idea, then.

What do you think about throwing IllegalArgumentException instead of JsonParseException from the write method?

[sebthom]

Thank you for verifying it. Looks like it was not a good idea, then.

What do you think about throwing IllegalArgumentException instead of JsonParseException from the write method?

I have no strong opinion about this. I am also fine the way it is.

[pisv]

Just looking through the GSON source code, it seems that the JsonParseException is thrown strictly from parse or read methods, as also specified in its Javadoc. That's why I'm not quite happy about throwing it from the write method here. It is a minor thing, of course.

[sebthom]

Just looking through the GSON source code, it seems that the JsonParseException is thrown strictly from parse or read methods, as also specified in its Javadoc. That's why I'm not quite happy about throwing it from the write method here. It is a minor thing, of course.

Does it have any practical relevance which one is thrown? Both are not recoverable or?

[pisv]

It does have a practical relevance by setting a precedent of throwing JsonParseException in write methods in the project's code base. By doing so, it sets an example that probably will be copied in the future.

Otherwise, we could just as well throw a RuntimeException (or any subclass of it) in this case.

[sebthom]

Maybe this should be discussed separately and addressed in a separate PR? I can see other places where JsonParseException is thrown in a similar manner, like in EitherTypeAdapter.create or MessageTypeAdapter.createMessage. So if this is changed than probably better everywhere at once.

[pisv]

Thank you for discussing it in detail. The only reason for this discussion is that I could find no prior cases of throwing JsonParseException from write (serialization) methods neither in GSON itself nor in LSP4J. (The methods you mentioned where JsonParseException is thrown are only called from read methods.) That's why I'm concerned that this PR is setting a precedent in throwing JsonParseException from a write method.

[sebthom]

I change it to whatever you feel comfortable. Shall I go for IllegalArgumentException?

[pisv]

I'd suggest throwing IllegalArgumentException instead of JsonParseException in this case. So, yes, please. Thank you!

[sebthom]

@pisv done

[pisv]

@sebthom I have made minor updates to ResourceOperationTypeAdapterTest, mostly removed references to 'current buggy implementation`, which is no longer buggy. Please have a look and if you are fine with my changes, I'll approve and merge this PR. Thanks!

[sebthom]

Looks good. Thanks!

[pisv]

LGTM, thanks! 👍 And sorry for the extensive discussion of a small thing that mattered to me.