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

Allow use of java.nio.file.Path for readValue(), writeValue() #2013

Closed
XakepSDK opened this issue Apr 26, 2018 · 17 comments
Closed

Allow use of java.nio.file.Path for readValue(), writeValue() #2013

XakepSDK opened this issue Apr 26, 2018 · 17 comments
Labels
3.x Issues to be only tackled for Jackson 3.x, not 2.x
Milestone

Comments

@XakepSDK
Copy link
Contributor

Version 2.9.5
There is no out of box method to read from Path and write to Path, it will be usefull to make these methods.

@cowtowncoder
Copy link
Member

cowtowncoder commented Apr 26, 2018

java.nio.Path should be supported: serialization via StringLikeSerializer, and deserialization via FromStringDeserializer.
Do you have a unit test to show this failing?

@XakepSDK
Copy link
Contributor Author

XakepSDK commented Apr 26, 2018

No, i'm not about serialization/deserialization, i'm about readValue and writeValue methods, there is readValue(File) and writeValue(File, Object), but no readValue(Path), writeValue(Path, Object).
Sorry for wrong info in first comment.

@cowtowncoder cowtowncoder changed the title readValue/writeValue nio Path support Allows use of java.nio.file.Path for readValue(), writeValue() Apr 26, 2018
@cowtowncoder
Copy link
Member

@XakepSDK Ah! Yes that makes more sense.

Since this is API addition, will need to go in 3.0. But makes sense, the only (?) concern is large number of potential overloads. But I can see whether it should be added to just ObjectReader / ObjectWriter, or for ObjectMapper too.

@cowtowncoder cowtowncoder added the 3.x Issues to be only tackled for Jackson 3.x, not 2.x label Apr 26, 2018
@michael-o
Copy link

michael-o commented Dec 21, 2019

I know nothing about the innards of Jackson, but you cannot deserialize Path because it is an interface and you'd need the FileSystem instance for. You cannot even transport between Windows and Un*x.

@cowtowncoder
Copy link
Member

@michael-o I think this would not be about deserializing Path instances, but allowing their use as source to read from.

@michael-o
Copy link

@cowtowncoder Are you certain? The discussion is about serializers and not JSON readers.

@XakepSDK
Copy link
Contributor Author

@michael-o cowtowcoder is correct

@michael-o
Copy link

You should improve your description to make this clear.

@sdoeringNew
Copy link

sdoeringNew commented Feb 18, 2021

Is this a big change?

As Path and File are arbitrary interchangeable one method might simply be a delegate to the other method:

public void writeValue(File resultFile, Object value) throws IOException, JsonGenerationException, JsonMappingException {
    this._configAndWriteValue(this._generatorFactory.createGenerator(resultFile, JsonEncoding.UTF8), value);
}

public void writeValue(Path resultPath, Object value) throws IOException, JsonGenerationException, JsonMappingException {
    writeValue(resultPath.toFile(), value);
}

@cowtowncoder cowtowncoder added 2.13 and removed 3.x Issues to be only tackled for Jackson 3.x, not 2.x labels Feb 18, 2021
@cowtowncoder
Copy link
Member

@sdoeringNew No, it is not a difficult thing to do at all -- the only possible concern is that the API size for ObjectMapper is already rather big. But even there these could be just added for ObjectReader / ObjectWriter.
Originally this was requested when Jackson 2.9 had been released and expectation was that we'd go to 3.0 after 2.9, but situation has changed -- we are developing 2.13 now (3.0 is still work-in-progress, concurrently) -- so this addition could go in 2.13.
I just haven't had time to consider this. But it would be a relatively simple thing for PR, I think, unless I miss something.

@sdoeringNew
Copy link

Great. I'd be happy to add the PR. 👍

@michaelhixson
Copy link
Contributor

Ideally the new methods would not use Path#toFile() because that method throws UnsupportedOperationException if the path is not on the default file system. Examples of non-default file systems include zip file systems and in-memory file systems.

I maintain an application that uses Jackson to interact with both of those types of file systems. If you added methods that accept Path objects but invoke toFile() on them, I wouldn't be able to use any of those methods.

I'd think it would be better if the Path-accepting methods delegate to {Input,Output}Stream-accepting ones. For example:

public void writeValue(Path resultPath, Object value) throws IOException, JsonGenerationException, JsonMappingException {
    try (OutputStream out = Files.newOutputStream(resultPath)) {
        writeValue(out, value);
    }
}

@sdoeringNew
Copy link

Yeah, well, if I can't use toFile() then there is a different problem.

If I want to do it right and neat like this:

public <T> T readValue(Path src, Class<T> valueType)
    throws IOException, StreamReadException, DatabindException
{
    _assertNotNull("src", src);
    return (T) _readMapAndClose(_jsonFactory.createParser(src), _typeFactory.constructType(valueType));
}

The TokenStreamFactory has to be updated in the jackson-core project. But that project is still at Java 1.6. Path came at 1.7.


Of course I could delegate it:

public <T> T readValue(Path src, Class<T> valueType)
    throws IOException, StreamReadException, DatabindException
{
    _assertNotNull("src", src);
    return readValue(Files.newInputStream(src), valueType);
}

But that seems rather ugly if there is a whole factory dedicated to this kind of operation.
(Btw. the ObjectReader does this ugly thing, but not the ObjectWriter.)


Ideas?

@michaelhixson
Copy link
Contributor

But that seems rather ugly if there is a whole factory dedicated to this kind of operation.

I'm not following this part. What factory do you mean?

The second readValue you shared looks fine to me, except I thought you would need to close the InputStream.

My reading of _jsonFactory.createParser((File) src) is that it converts the file into an InputStream, and that it ends up reading that InputStream in the same way that your method does. The only difference I can see is that the File-accepting _jsonFactory.createParser could produce better exception messages than the InputStream-accepting one (it attaches the input object to the context for that purpose, and File has a helpful toString() whereas InputStream generally does not).

@cowtowncoder
Copy link
Member

Yes, jackson-core is still JDK 6 only; even if jackson-databind is now moving to JDK 8 for Jackson 2.13 (and had JDK7 for a while now). Jackson 3.0 has JDK 8 as baseline for all components, so specific version for TokenStreamFactory could be added there if that is useful (I have no strong opinion).

So I guess it is possible to have different implementation for 2.13 vs 3.0 -- I would start with 2.13.

sdoeringNew added a commit to sdoeringNew/jackson-databind that referenced this issue Feb 22, 2021
sdoeringNew added a commit to sdoeringNew/jackson-databind that referenced this issue Feb 22, 2021
@sdoeringNew
Copy link

I've started with 3.0 and added pull requests for jackson-core and jackson-databind.

I'm unsure about adding it to 2.13, though. It would be a half-heartedly support for Path after the JDK has the nio-package ("new" IO) for ten years now. This issue had the 3.0-label three years and it still can be 3.0. 😏

But if you say so I'll add it "workaround-ish" to 2.13.

sdoeringNew added a commit to sdoeringNew/jackson-databind that referenced this issue Feb 26, 2021
@cowtowncoder cowtowncoder added 3.x Issues to be only tackled for Jackson 3.x, not 2.x and removed 2.13 labels Feb 27, 2021
@cowtowncoder cowtowncoder added this to the 3.0.0 milestone Feb 27, 2021
@cowtowncoder cowtowncoder changed the title Allows use of java.nio.file.Path for readValue(), writeValue() Allow use of java.nio.file.Path for readValue(), writeValue() Feb 27, 2021
@cowtowncoder
Copy link
Member

Merged to master for 3.0.0, thank you @sdoeringNew!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3.x Issues to be only tackled for Jackson 3.x, not 2.x
Projects
None yet
Development

No branches or pull requests

5 participants