Unnecessary JsonProcessingException Catch clause in ObjectMapper.writeValueAsString(Object value)

[hperperidis]

In ObjectMapper class, method writeValueAsString(Object value) includes a throw declaration of type JsonProcessingException.

Current Method code (minus comments) - Latest release

	@SuppressWarnings("resource")
	public String writeValueAsString(Object value) throws JsonProcessingException {
		// alas, we have to pull the recycler directly here...
		SegmentedStringWriter sw = new SegmentedStringWriter(_jsonFactory._getBufferRecycler());
		try {
			_configAndWriteValue(_jsonFactory.createGenerator(sw), value);
		} catch (JsonProcessingException e) {
			throw e;
		} catch (IOException e) { // shouldn't really happen, but is declared as
									// possibility so:
			throw JsonMappingException.fromUnexpectedIOE(e);
		}
		return sw.getAndClear();
	}

The _configAndWriteValue() call in the snippet above (surrounded by the try - catch block, is only throwing an IOException, which is caught and handled in the seccond catch clause.
So the only other part of this try - catch block that could potentially throw the JsonProcessingException is the _jsonFactory.createGenerator(sw) call, where sw is the SegmentedStringWriter which extends Writer.

Looking at how the JsonFactory instance is populated in this class, we can see in the Object mapper constructor:

	public ObjectMapper() {
		this(null, null, null);
	}

which calls:

	public ObjectMapper(JsonFactory jf, DefaultSerializerProvider sp, DefaultDeserializationContext dc) {
		/*
		 * 02-Mar-2009, tatu: Important: we MUST default to using the mapping
		 * factory, otherwise tree serialization will have problems with
		 * POJONodes. 03-Jan-2010, tatu: and obviously we also must pass 'this',
		 * to create actual linking.
		 */
		if (jf == null) {
			_jsonFactory = new MappingJsonFactory(this);
		} else {
			_jsonFactory = jf;
			if (jf.getCodec() == null) { // as per [JACKSON-741]
				_jsonFactory.setCodec(this);
			}
		}
		_subtypeResolver = new StdSubtypeResolver();
		RootNameLookup rootNames = new RootNameLookup();
		// and default type factory is shared one
		_typeFactory = TypeFactory.defaultInstance();

		SimpleMixInResolver mixins = new SimpleMixInResolver(null);
		_mixIns = mixins;
		BaseSettings base = DEFAULT_BASE.withClassIntrospector(defaultClassIntrospector());
		_configOverrides = new ConfigOverrides();
		_serializationConfig = new SerializationConfig(base, _subtypeResolver, mixins, rootNames, _configOverrides);
		_deserializationConfig = new DeserializationConfig(base, _subtypeResolver, mixins, rootNames, _configOverrides);

		// Some overrides we may need
		final boolean needOrder = _jsonFactory.requiresPropertyOrdering();
		if (needOrder ^ _serializationConfig.isEnabled(MapperFeature.SORT_PROPERTIES_ALPHABETICALLY)) {
			configure(MapperFeature.SORT_PROPERTIES_ALPHABETICALLY, needOrder);
		}

		_serializerProvider = (sp == null) ? new DefaultSerializerProvider.Impl() : sp;
		_deserializationContext = (dc == null)
				? new DefaultDeserializationContext.Impl(BeanDeserializerFactory.instance) : dc;

		// Default serializer factory is stateless, can just assign
		_serializerFactory = BeanSerializerFactory.instance;
	}

This constructor initializes the JsonFactory by default, as an instance of its MappingJsonFactory implementing subclass.

Looking at this class, we can see it does not actually overide the createGenerator(Writer w) method from com.fasterxml.jackson.core.JsonFactory.

And from com.fasterxml.jackson.core.JsonFactory we can see that all overloaded instances of the createGenerator method are not throwing a JsonProcessingException but rather again an IOException (See link https://github.com/FasterXML/jackson-core/blob/master/src/main/java/com/fasterxml/jackson/core/JsonFactory.java).

So the catch clause for JsonProcessingException can be completely removed from writeValueAsString in ObjectMapper.

Recomended Change

	@SuppressWarnings("resource")
	public String writeValueAsString(Object value) throws JsonProcessingException {
		// alas, we have to pull the recycler directly here...
		SegmentedStringWriter sw = new SegmentedStringWriter(_jsonFactory._getBufferRecycler());
		try {
			_configAndWriteValue(_jsonFactory.createGenerator(sw), value);
		}  catch (IOException e) { // shouldn't really happen, but is declared as
									// possibility so:
			throw JsonMappingException.fromUnexpectedIOE(e);
		}
		return sw.getAndClear();
	}

Unless I have missed something, I think the above refactor could be made without causing serious issues.

[cowtowncoder]

The intent is to avoid unnecessary wrapping of JsonProcessingExceptions. And since it is sub-class of IOException, some mechanism is needed to avoid that: either by two catch clauses, or check within one catch.

[theangrydev]

The comments by @hperperidis are pointing out that he cannot see any code path that produces a JsonProcessingException. Is this correct or not? Even if it is correct, would you still want to leave the extra catch there "just in case" for the future..?

[hazeltroost]

Creating a JsonProcessingException isn't likely, but it's easy to court if wanted.

@Test
public void forceJsonParseException() {
    try {
        Object mockItem = mock(Object.class);
        when(mockItem.toString()).thenReturn(mockItem.getClass().getName());
        new ObjectMapper().writeValueAsString(mockItem);
        fail("did not throw JsonProcessingException");
    } catch (JsonProcessingException e) {
        //pass
    }
}

[ruckc]

@cowtowncoder - Please reopen this. By rethrowing the exception (instead of wrapping it with another exception), is that I can't see why i'm getting "Current context not Object but root". Yes i'm probably doing something wrong to get this error, but with the throw e line, I can't see the original JsonProcessingException's stack trace. https://www.baeldung.com/java-wrapping-vs-rethrowing-exceptions

Ideally, just throw new JsonProcessingException(e);

[yawkat]

@ruckc the rethrowing behavior will not replace the stack trace. you will still have the old stack trace available.

[cowtowncoder]

@ruckc If you want a change, please file a new issue, explaining exact issue you have. It does not sound the original issue and I think it is confusing to reopen issues after the fact in such cases.
But I suspect @yawkat's comment is valid here as well: re-throwing an exception does not do anything to stack trace -- if something is not available it's due to something else.

But I would specifically need a reproduction of your problem, not just general description.