Skip to content

Conversation

@laeubi
Copy link
Member

@laeubi laeubi commented Jul 27, 2025

I noticed that when working on eclipse-pde/eclipse.pde#1898 that we need special handling for reading/writing a IPath.

As IPath is just a collection of strings and some flags I don't see why we should not support it even though it might not be portable (in wich case one would better use an own serialization way).

@merks
Copy link
Contributor

merks commented Jul 27, 2025

I think this is maybe a poor idea. Making something serializable does lead to the assumption that you can serialize and deserialize it anywhere. So while java.net.URI is Serializable, java.nio.file.Path is not. I can see that it might be convenient in some use cases, I think it's potentially misleading. We should solicit more opinions on the matter.

@github-actions
Copy link

github-actions bot commented Jul 27, 2025

Test Results

  807 files  ±0    807 suites  ±0   1h 18m 5s ⏱️ + 1m 11s
2 200 tests ±0  2 151 ✅ ±0   49 💤 ±0  0 ❌ ±0 
6 612 runs  ±0  6 463 ✅ ±0  149 💤 ±0  0 ❌ ±0 

Results for commit 3726701. ± Comparison against base commit 5c20643.

♻️ This comment has been updated with latest results.

@laeubi
Copy link
Member Author

laeubi commented Jul 27, 2025

I think this is maybe a poor idea. Making something serializable does lead to the assumption that you can serialize and deserialize it anywhere.

What will work ... but of course a file path (what can be an URI) might not exist.

java.nio.file.Path is not.

I think this is more because you cannot restore a (generic) path without the provider but IPath itself has no different providers.

@laeubi laeubi force-pushed the make_serializable branch from 04e44e9 to 3726701 Compare July 27, 2025 08:05
@laeubi
Copy link
Member Author

laeubi commented Jul 27, 2025

I'm now using to/fromPortablePath to write / restore the object, @merks does it address your concerns?

@merks
Copy link
Contributor

merks commented Jul 27, 2025

I suppose, that seems better.

But when working on Sundays it's still a good idea to give other "normal" committers (who don't work on Sundays) an opportunity to express any concerns they might have. 😕

@laeubi
Copy link
Member Author

laeubi commented Jul 27, 2025

This is nothing urgent ... but seems to remove the need for workarounds in some places and does not hurt from my perspective.

Copy link
Member

@HannesWell HannesWell left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

java.nio.file.Path is not.

I think this is more because you cannot restore a (generic) path without the provider but IPath itself has no different providers.

That's one reasons and another reason might be that internally Path is also a wrapper around a String using the Platform's native path element separator, which is different on Unix (/) and Windows (\).

But in general I don't see general problems in making IPath serializable as a file with specific a Path can be removed or altered at any point in time (e.g. by another program). So that it might not exist at de-serialization is something that definitively has to be considered.

return new PortablePath(toPortableString());
}

static final class PortablePath implements Serializable {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can't we use a simple record here? And if not, can't a regular serialVersionUID be used?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is a record simpler?
And what is a "regular" serialVersionUID or more why is one not a regular one?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is a record simpler?

Well, the usual: Less boiler-plate.
But thinking about it a second time, using Externalizable could be even simpler and would require no magic methods
https://www.baeldung.com/java-externalizable

And what is a "regular" serialVersionUID or more why is one not a regular one?

One is the default ID and should only be used with custom serialization logic and I think this 'inner value holder' uses the 'generated' serialization mechanism and therefore should have a generated non-default ID:
grafik

But that's also obsolete if Externalizable is used.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But thinking about it a second time, using Externalizable could be even simpler and would require no magic methods

Externalizable does not work here as we have all fields final in the class (on purpose).

One is the default ID and should only be used with custom serialization logic

How more custom can it be here? Beside that any number would suffice here, having a more random looking one did not really help here but if a generated one would be preferred why not ... its just a number no one ever will look at really.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Isn't the generated number somehow dependent on the implementation class details such that if you change it you can generate a new one?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Isn't the generated number somehow dependent on the implementation class details such that if you change it you can generate a new one?

Java generates one by default based on the classfile and I would assume JDT is using that one algorithm when asking to add one explicitly (!).

If incrementing a number from 1 > 2 is better or deleting the value (hopefully never forget :-P ) and let it make the default explicit again is something I could not judge.

Usually one want to have a stable one so recompilation does not change it and making everything previously saved invalid even if the class structure is not changed.

So usually:

  1. any number is sufficient
  2. you should change it to something else if you change the structure and can't live e.g. with new fields becoming null
  3. If you don't supply one you will get some default.

@laeubi
Copy link
Member Author

laeubi commented Jul 31, 2025

@tjwatson any thoughts?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants