chore: Refactor x509Provider to create a shared Utils class for Mtls#1907
chore: Refactor x509Provider to create a shared Utils class for Mtls#1907vverman wants to merge 7 commits intogoogleapis:mainfrom
Conversation
vverman
force-pushed
the
refactor-x509-provider
branch
from
1500bea to
f78ed98
Compare
| public X509Provider( | ||
| EnvironmentProvider envProvider, | ||
| PropertyProvider propProvider, | ||
| String certConfigPathOverride) { |
There was a problem hiding this comment.
we can leave this in a future refactor PR if this is a lot of work. Thoughts on creating a builder? Constructor only takes in the builder and we won't have to change the constructor signature in the future.
This was envProvider and propProvider can be set in the builder to be package-private for the tests. Only certConfigPathOverride would be public
There was a problem hiding this comment.
Thanks for the suggestion.
I think we can leave this one out for now because I don't think the argument list is big enough to justify the Builder's inclusion and we don't anticipate any changes to X509Provider (in the short-term).
In case this changes, we can implement it then.
There was a problem hiding this comment.
Ok, let's keep as-is. I can look into this later when I get more time and change this as this is marked with InternalApi.
| @@ -0,0 +1,10 @@ | |||
| package com.google.auth.oauth2; | |||
There was a problem hiding this comment.
add header here.
The EnvProvider and PropertyProvider classes that are public because they're used between the mtls/ and oauth2/ folders. Can you add @InternalApi?
There was a problem hiding this comment.
bumping. missing a header here as well
| * | ||
| * <p>For internal use only. | ||
| */ | ||
| public class SystemEnvironmentProvider implements EnvironmentProvider, Serializable { |
There was a problem hiding this comment.
qq, what is the difference between SystemEnvironmentProvider and EnvironmentProvider?
There was a problem hiding this comment.
An EnvProvider is an interface which can be implemented by test-classes to pass in their own env variables.
The SystemEnvProvider implements this EnvProvider and implements the associated functions such that the env-variables are fetched from the system env.
There was a problem hiding this comment.
Got it. Can you add @internalapi to this class and for SystemPropertyProvider?
Since this is a refactor, I'm going to change the title to |
lqiu96
changed the title
|
E
El martes, 24 de marzo de 2026, ***@***.***> escribió:
… ***@***.**** commented on this pull request.
------------------------------
In oauth2_http/java/com/google/auth/mtls/X509Provider.java
<#1907 (comment)>
:
> + public X509Provider(
+ EnvironmentProvider envProvider,
+ PropertyProvider propProvider,
+ String certConfigPathOverride) {
Thanks for the suggestion.
I think we can leave this one out for now because I don't think the
argument list is big enough to justify the Builder's inclusion and we don't
anticipate any changes to X509Provider (in the short-term).
In case this changes, we can implement it then.
—
Reply to this email directly, view it on GitHub
<#1907?email_source=notifications&email_token=B5MKPO3RH26N37TDJ35VCOL4SL3TJA5CNFSNUABKM5UWIORPF5TWS5BNNB2WEL2QOVWGYUTFOF2WK43UKJSXM2LFO4XTIMBQGIZTKMBWGUYKM4TFMFZW63VKON2WE43DOJUWEZLEUVSXMZLOOSWGM33PORSXEX3DNRUWG2Y#discussion_r2984325705>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/B5MKPOZWHPYIKAJ5GNPMM5L4SL3TJAVCNFSM6AAAAACW43RKUSVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHM2DAMBSGM2TANRVGA>
.
You are receiving this because you are subscribed to this thread.Message
ID: <googleapis/google-auth-library-java/pull/1907/review/4002350650@
github.com>
|
| @@ -0,0 +1,24 @@ | |||
| package com.google.auth.oauth2; | |||
| @@ -0,0 +1,22 @@ | |||
| package com.google.auth.oauth2; | |||
| import java.util.HashMap; | ||
| import java.util.Map; | ||
|
|
||
| public class TestPropertyProvider implements PropertyProvider { |
There was a problem hiding this comment.
qq, is this TestPropertyProvider used anywhere in the tests? If not, can we remove it?
3 participants
Fixes #1745
Addressed a concern raised by Andy refer.
Now X509Provider only exposes the necessary methods needed by the MtlsProvider interface