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

Reflection is broken on the JavaPlugin instance as it is instantiated as a proxy class #755

Open
2 tasks done
zodiia opened this issue Apr 19, 2023 · 5 comments · May be fixed by #1003
Open
2 tasks done

Reflection is broken on the JavaPlugin instance as it is instantiated as a proxy class #755

zodiia opened this issue Apr 19, 2023 · 5 comments · May be fixed by #1003
Labels
bug-exception Issue describes a bug in MockBukkit that prevent plugins from being tested. help wanted

Comments

@zodiia
Copy link
Contributor

zodiia commented Apr 19, 2023

Is there an existing issue for this?

  • I have searched the existing issues

Are you using the latest version of MockBukkit?

  • I am using the latest version of MockBukkit.

Minecraft Version

1.19

Describe the bug

Since updating to Paper 1.19.4, and MockBukkit to its latest version, most of my tests broke because some of the initialization logic in the plugin class uses its class instance to do reflection.

With this update, some of the logic for initializing a plugin has been changed, and in particular those lines: https://github.com/MockBukkit/MockBukkit/pull/747/files#diff-4a7fdcf888c1e81d04a702b47342e96a583b026f1036b44599ab74e4e91ceb38R83-R89. With this, the plugin instance is no longer an instance of the actual plugin class, but rather of a proxy class that extends the plugin class. The result is that most reflection don't work the way it used to.

In particular, for my use case, I use the plugin's package name to find classes in subpackages and automagically inject them in my plugin's runtime. However, the package name is now empty, and it broke.

I could do a workaround by checking it on the superclass instance, but that would imply adding checks for all usage of reflection on this class for whether this is a test environment or not, and that isn't ideal.

Reproducible Test

  • Create a new Paper project on version 1.19.4 and with MockBukkit
  • Add a test and load the plugin
  • Check this plugin's package name using instance.getClass().getPackageName()
  • See that the package name is an empty string

Anything else?

I don't know ByteBuddy enough to fix this issue myself, I don't know how to "transfer" all the reflection data to the proxy instance, but I'm hoping that someone with more ByteBuddy knowledge than me will be able to.

@zodiia
Copy link
Contributor Author

zodiia commented Apr 19, 2023

I should probably mention that, while the example I'm giving is about package names, this is only an example and this issue is occurring in other places.

@Insprill Insprill added help wanted bug-exception Issue describes a bug in MockBukkit that prevent plugins from being tested. labels Apr 19, 2023
@seeseemelk
Copy link
Member

We use the proxy to get control over how and which classloader loads the 'main' plugin class. Paper changed some internals that we were abusing to work around needing.

This change should have been marked as a 3.0.0 release in order to indicate that breakages are somewhat expected, but the pipeline broke in an unexpected way causing it to be released as part of the 2.x.y series.

It's probably possible in some way to make byte buddy/the classloader believe that the proxy class is in the same package as the original. Would such a solution fix your use case?

Alternatively, perhaps you can change your tests so that they use the parent class instead of the plugin class directly?

@zodiia
Copy link
Contributor Author

zodiia commented May 10, 2023

I saw what happened with the 3.0.0 release, but don't worry it didn't really affect me.

Tricking the classloader and bytebuddy into thinking the proxy class is in the same package as the original would indeed fix my issue. But I don't think it would be a very good fix, as it would only fix my very specific problem, and potentially won't fix other issues that would involve reflections over that proxy class.

I did the workaround you're describing in my tests, and it works.

Now that I think about it, wouldn't it be possible to simply replace the getClass of the proxy class and return the getClass of the parent class, and (hopefully) that would magically work?

@dumptruckman
Copy link

dumptruckman commented Aug 31, 2023

Chiming in that this is impacting Multiverse as well. We are using dependency injection and similarly rely on reflection that breaks because of the use of the proxy.

@zodiia zodiia changed the title Updating to 1.19.4 broke code based on reflection with the JavaPlugin Reflection is broken on the JavaPlugin instance as it is a proxy class Jan 11, 2024
@zodiia zodiia changed the title Reflection is broken on the JavaPlugin instance as it is a proxy class Reflection is broken on the JavaPlugin instance as it is instantiated as a proxy class Jan 11, 2024
@Thorinwasher
Copy link
Contributor

Thorinwasher commented May 1, 2024

I've looked into this a bit, I almost have a solution for this; I am able to change the class contextClassLoader for the thread that solves the test with my own implementation of a class loader. The problem comes with the actuall implementation (I believe at least).

The problem from what I can see is that the classloader that is going to be used when loading a new class is inherited from the current class as written here. This means that I can not go back to using another classloader for loading a class, as all class-loading instansiated after loading that class will also be loaded using that other class loader.

That seems fine by me; this far it is seems possible to just load everything from our own custom class loader. Here's where the issues arise; some classes has been loaded by the previous classloader (the classloader which we replaced wih our own implementation). Any class which is inheriting from that class has to be linked to that class, which only can be done using the previous class loader. So to say it is necessary to return the same class instance of any class that has already been loaaded before the class loader was changed.

Okay, and from that, one might think, why not look into the previous class loader and check if the class already has been loaded there?

This is not possible since Java 9 unfortunately, as the method that does that has a protected access modifier, which can not be changed as ClassLoader is a core java method.

(I made a draft pull request for this, see #1003)

@Thorinwasher Thorinwasher linked a pull request May 1, 2024 that will close this issue
2 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug-exception Issue describes a bug in MockBukkit that prevent plugins from being tested. help wanted
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants