jackson-core shading of ch.randelshofer:fastdoubleparser

[Josh Curry]

It was recently discovered another library is using one of the fastdouble shaded classes from jackson-core.  

https://github.com/aws/event-ruler/blob/main/src/main/software/amazon/event/ruler/ByteMachine.java#L3

Since the relocated package is not included in the module-info, it seems pretty clear that there was no intention for downstream consumers to be able use this code.  However, there is no way to prevent it when not using modules, and usage of shaded code is a common problem, which is often done by accident.  Because of this and tendency to bloat application sizes, I generally find shading more trouble than its worth. However,  it can be useful sometimes, and one approach is to randomize the package name of the relocated code in order to make consumption more obviously frowned upon as well as more difficult. 

Is there a specific reason for shading this dependency other than not wanting to expose a new dependency to consumers of jackson-core?  If not, then might be easier or clearer to expose the dependency in the next release - 2.18. 

Thoughts?

[Tatu Saloranta]

On Fri, Apr 12, 2024 at 2:09 PM Josh Curry <brane...@gmail.com> wrote:
>
> It was recently discovered another library is using one of the fastdouble shaded classes from jackson-core.
>
> https://github.com/aws/event-ruler/blob/main/src/main/software/amazon/event/ruler/ByteMachine.java#L3
>
> Since the relocated package is not included in the module-info, it seems pretty clear that there was no intention for downstream consumers to be able use this code. However, there is no way to prevent it when not using modules, and usage of shaded code is a common problem, which is often done by accident. Because of this and tendency to bloat application sizes, I generally find shading more trouble than its worth. However, it can be useful sometimes, and one approach is to randomize the package name of the relocated code in order to make consumption more obviously frowned upon as well as more difficult.

Right, this is a problem. But would package name change help? My
experience has been its IDEs that are the reason for exposing shaded
packages, and that tends to be based on class names and package names
are all but ignored (by tools and -- alas! -- developers).

Once upon a time I had a clever (so I thought) way of preventing such
accidental imports: I added `private` as part of package name -- like
"com.jackson.core.private.shaded.xxxx" since it's actually legal in
package name, but illegal in source code ;-)
So one could not get auto-completed import statements to work.

But some people strongly protested for some reason I don't even recall
any longer (probably some tool somewhere having heartburn despite
being legal usage at bytecode/JVM/JDK level) so I went back to some
legal identifier instead.

> Is there a specific reason for shading this dependency other than not wanting to expose a new dependency to consumers of jackson-core? If not, then might be easier or clearer to expose the dependency in the next release - 2.18.
>
> Thoughts?

Shading is done mostly to keep jackson-core as a
zero-runtime-dependency library, and I do not want to give up that
property.
Second benefit is that this prevents version incompatibilities between
jackson-core and fast-double-parser.

So: I'd be fine improving ways that reduce the likelihood of
accidental (or intentional for that matter) use of FDP via
jackson-core.
But I do not see it beneficial to change shading into regular
dependency: that seems to have more downsides for regular users.
And while I do not want to set anyone up to failure, I also have only
little sympathy for those who use transitive shaded dependencies --
developers really should know what they are using and from where.
Automatically click-click-clicking IDE suggestions is dangerous habit
and can lead to all kinds of issues with unwanted dependencies.

Actually, come to think of that -- I do not think that change to
explicit regular dependencies would be that much better at all, wrt
versioning problems. It is still possible to (and maybe even more so)
to rely on FDP transitively; and whenever Jackson upgrades its
version, downstream usage may well break.

-+ Tatu +-

[Josh Curry]

> But would package name change help?

I think it would - the current package name is very easy to infer to be part of jackson-core.  

> Shading is done mostly to keep jackson-core as a

zero-runtime-dependency library

Yes, agreed that this might be useful.  

> I do not think that change to explicit regular dependencies would be that much better at all, wrt

versioning problems.It is still possible to (and maybe even more so)

to rely on FDP transitively; and whenever Jackson upgrades its
version, downstream usage may well break.

This is a general problem though for any dependency that someone takes - including jackson-*.  At some point the application owner has to make a choice on which version of a dependency they take in the event of a conflict.  And yes, if there is little trust that the dependency will provide backward compatible upgrades, then maybe it makes sense to shade - or perhaps find a different dependency that is more reliable.  Also, when fastdoubleparser was shaded, it was a dot release - so the likelihood of changes was higher.  However, now there is a 1.0 release, so theoretically it will be more stable.  

--
You received this message because you are subscribed to the Google Groups "jackson-user" group.
To unsubscribe from this group and stop receiving emails from it, send an email to jackson-user...@googlegroups.com.
To view this discussion on the web visit https://groups.google.com/d/msgid/jackson-user/CAGrxA24oK3mDW6WDm3jdN%2BMV%3DWybi0_ad2kUfrXjeOr%2BWs88Jw%40mail.gmail.com.

[Tatu Saloranta]

On Fri, Apr 12, 2024 at 8:28 PM Josh Curry <brane...@gmail.com> wrote:
>
> > But would package name change help?
>
> I think it would - the current package name is very easy to infer to be part of jackson-core.

So the suggestion would be to use a package name outside of
"com.fasterxml.jackson"?
I guess that is a possibility, although namespacing in itself is used
to reduce the chance of name collision
(so shaded version won't conflict with any potential external package).

What do you think would make sense?

>
> > Shading is done mostly to keep jackson-core as a
> zero-runtime-dependency library
>
> Yes, agreed that this might be useful.
>
>
> > I do not think that change to explicit regular dependencies would be that much better at all, wrt
> versioning problems.It is still possible to (and maybe even more so)
> to rely on FDP transitively; and whenever Jackson upgrades its
> version, downstream usage may well break.
>
> This is a general problem though for any dependency that someone takes - including jackson-*. At some point the application owner has to make a choice on which version of a dependency they take in the event of a conflict. And yes, if there is little trust that the dependency
> will provide backward compatible upgrades, then maybe it makes sense to shade - or perhaps find a different dependency that is more reliable. Also, when fastdoubleparser was shaded, it was a dot release - so the likelihood of changes was higher. However, now there is a 1.0 release, so theoretically it will be more stable.

Be that as it may; at this point I prefer keeping FDP shaded.

I am +0 on changing shaded package name prefix, but could be convinced
to do that with a good suggestion.

-+ Tatu +-

> To view this discussion on the web visit https://groups.google.com/d/msgid/jackson-user/CAOSep7JeRjfgAvKZs-YeugcyqU7V0dy3LmK6U8NVi8iD%3DJqQ_w%40mail.gmail.com.

[Josh Curry]

> What do you think would make sense?

How about including something like "internal.${version}" before doubleparser?

So for 2.18.0- it would be: 

com/fasterxml/jackson/core/internal/2/18/0/doubleparser/

And/or including "shadow" or "shaded" could also help minimize accidental use downstream.  

com/fasterxml/jackson/core/internal/2/18/0/shadow/doubleparser/

To view this discussion on the web visit https://groups.google.com/d/msgid/jackson-user/CAL4a10jSXq6xpyRwErtzoZ8bUgEzHOk4VHPTMoNptOvOqcNoJw%40mail.gmail.com.

[Tatu Saloranta]

On Mon, Apr 15, 2024 at 5:27 PM Josh Curry <brane...@gmail.com> wrote:
>
> > What do you think would make sense?
>
> How about including something like "internal.${version}" before doubleparser?
>
> So for 2.18.0- it would be:
>
> com/fasterxml/jackson/core/internal/2/18/0/doubleparser/
>
> And/or including "shadow" or "shaded" could also help minimize accidental use downstream.
>
> com/fasterxml/jackson/core/internal/2/18/0/shadow/doubleparser/

I like this idea! Suitably fragile for anyone who might add a dependency :)
But are numbers legal package path pieces?

Would you mind filing a Github issue for `jackson-core` for this?
I am not 100% sure how to split the version number in pom.xml, but I
suspect there are ways to
do that.

-+ Tatu +-

> To view this discussion on the web visit https://groups.google.com/d/msgid/jackson-user/CAOSep7JH6NZ_OuF8syjO9mqcE7pFcoOtpT-WZJE4PjKEhQyF-Q%40mail.gmail.com.

[Josh Curry]

> But are numbers legal package path pieces?

Alas, no - has to start with a letter and sadly https://plugins.gradle.org/plugin/com.github.johnrengelman.shadow  doesn't perform any validation on the package name.  

Provided a similar suggestion in the issue that is a valid package name:

https://github.com/FasterXML/jackson-core/issues/1264