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

feat: Add generic proxy implementation #11201

Closed
wants to merge 2 commits into from

Conversation

mykola-mokhnach
Copy link

Description

This is my proposal to solve appium/java-client#1694 (comment)

Motivation and Context

There is no easy way in Java to patch/instrument a class if it is already loaded. Java Agent mechanism allows that, but it requires users to change the way their jar is being executed and is pretty complicated.
This implementation allows to create a generic proxy for any class and add one or more listeners to public method invocations. The current decorators implementation stays untouched though. I was not able to find an acceptable way to modify it in a non-breaking manner to support the current proxying strategy.

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

cc @valfirst @pujagani

@CLAassistant
Copy link

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
You have signed the CLA already but the status is still pending? Let us recheck it.

@sonarcloud
Copy link

sonarcloud bot commented Nov 1, 2022

SonarCloud Quality Gate failed.    Quality Gate failed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 7 Code Smells

0.0% 0.0% Coverage
0.0% 0.0% Duplication

@titusfortner
Copy link
Member

I'm concerned about adding things to the Support package, especially things that are specific to one language. Would it be an option for this to get bundled into a separate jar in a separate repo?

@mykola-mokhnach
Copy link
Author

I'm concerned about adding things to the Support package, especially things that are specific to one language. Would it be an option for this to get bundled into a separate jar in a separate repo?

No problem. I'll more the pr to Appium java client

@titusfortner
Copy link
Member

Well, mine is just one opinion, I was curious if it might make sense to do it independently. :)

@diemol what do you think about this in Selenium vs Appium (vs separate package)?

@pujagani
Copy link
Contributor

pujagani commented Nov 3, 2022

@mykola-mokhnach Thank you for your contribution. Can you help me understand how will this proxy be used to solve appium/java-client#1694 (comment)? How can it be used by Selenium? Just trying to understand where the code should sit. Do you have any follow-up PRs in mind that touches on the current decorators implementation?

@titusfortner We have webdriver decorator in the support package for Java. That functionality is specific to Java. So on that point, I don't see an issue. However, I want to first understand how this proxy will be used since it seems to address a broader Java concern and for that separating out is a good idea.

@mykola-mokhnach
Copy link
Author

@mykola-mokhnach Thank you for your contribution. Can you help me understand how will this proxy be used to solve appium/java-client#1694 (comment)? How can it be used by Selenium? Just trying to understand where the code should sit. Do you have any follow-up PRs in mind that touches on the current decorators implementation?

@pujagani Please check appium/java-client#1790
I've also added some more documentation and examples there. I am not going to further develop any changes to the existing decorators implementation as it is a dead-end. It will not work for class types without breaking changes in the internal and external logic.

@mykola-mokhnach mykola-mokhnach deleted the proxy branch November 3, 2022 08:05
@pujagani
Copy link
Contributor

pujagani commented Nov 3, 2022

Thank you for sharing that PR. It has helped answer my questions.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants