You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@commons.apache.org by "rmannibucau (via GitHub)" <gi...@apache.org> on 2024/02/09 19:51:25 UTC
[PR] Drop reflection from ExtraFieldUtils [commons-compress]
rmannibucau opened a new pull request, #480:
URL: https://github.com/apache/commons-compress/pull/480
As of today to make [compress] run on native-image (GraalVM) you must reister all classes in IMPLEMENTATIONS map. With this PR this is no more needed and [compress] zip support comes OOTB.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Drop reflection from ExtraFieldUtils [commons-compress]
Posted by "garydgregory (via GitHub)" <gi...@apache.org>.
garydgregory commented on code in PR #480:
URL: https://github.com/apache/commons-compress/pull/480#discussion_r1485144279
##########
src/main/java/org/apache/commons/compress/archivers/zip/ExtraFieldUtils.java:
##########
@@ -379,11 +382,28 @@ public ZipExtraField onUnparseableExtraField(final byte[] data, final int off, f
* </p>
*
* @param c the class to register
+ *
+ * todo: "at_deprecated this registration is global and shared by all consumers.", find a way to make it specific.
Review Comment:
IMO:
- Remove the TODO, and/or
- Deprecate the method and make its replacement public.
##########
src/main/java/org/apache/commons/compress/archivers/zip/ExtraFieldUtils.java:
##########
@@ -379,11 +382,28 @@ public ZipExtraField onUnparseableExtraField(final byte[] data, final int off, f
* </p>
*
* @param c the class to register
+ *
+ * todo: "at_deprecated this registration is global and shared by all consumers.", find a way to make it specific.
Review Comment:
IMO:
- Remove the TODO, and/or,
- Deprecate the method and make its replacement public.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Drop reflection from ExtraFieldUtils [commons-compress]
Posted by "rmannibucau (via GitHub)" <gi...@apache.org>.
rmannibucau commented on code in PR #480:
URL: https://github.com/apache/commons-compress/pull/480#discussion_r1485036494
##########
src/main/java/org/apache/commons/compress/archivers/zip/ExtraFieldUtils.java:
##########
@@ -379,11 +382,28 @@ public ZipExtraField onUnparseableExtraField(final byte[] data, final int off, f
* </p>
*
* @param c the class to register
+ *
+ * todo: "at_deprecated this registration is global and shared by all consumers.", find a way to make it specific.
Review Comment:
Hi @garydgregory not a draft but we can't fix it since _now_ it is part of the public API so it is more a todo for next breaking version.
The best is likely to let the (extensions) map (lookup only, no need to type it `Map`) be wired in the zip archive directly and this to stay the default/fallback without any public registration API but this is a breaking change I don't want right now - issue occurs rarely, when user registers custom impl and [compress] is in a shared classloader (OSGi, [compress] in tomcat/lib with multiple webapps consuming it etc...).
So this will need to be revisited in a coming version but can't be right now so for me it is really a todo.
Hope it explains it clearly enough.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Drop reflection from ExtraFieldUtils [commons-compress]
Posted by "rmannibucau (via GitHub)" <gi...@apache.org>.
rmannibucau commented on code in PR #480:
URL: https://github.com/apache/commons-compress/pull/480#discussion_r1485215496
##########
src/main/java/org/apache/commons/compress/archivers/zip/ExtraFieldUtils.java:
##########
@@ -113,24 +116,24 @@ public ZipExtraField onUnparseableExtraField(final byte[] data, final int off, f
/**
* Static registry of known extra fields.
*/
- private static final Map<ZipShort, Class<?>> IMPLEMENTATIONS;
+ private static final Map<ZipShort, Supplier<ZipExtraField>> IMPLEMENTATIONS;
static {
- IMPLEMENTATIONS = new ConcurrentHashMap<>();
- register(AsiExtraField.class);
- register(X5455_ExtendedTimestamp.class);
- register(X7875_NewUnix.class);
- register(JarMarker.class);
- register(UnicodePathExtraField.class);
- register(UnicodeCommentExtraField.class);
- register(Zip64ExtendedInformationExtraField.class);
- register(X000A_NTFS.class);
- register(X0014_X509Certificates.class);
- register(X0015_CertificateIdForFile.class);
- register(X0016_CertificateIdForCentralDirectory.class);
- register(X0017_StrongEncryptionHeader.class);
- register(X0019_EncryptionRecipientCertificateList.class);
- register(ResourceAlignmentExtraField.class);
+ IMPLEMENTATIONS = new HashMap<>(); // it can't be used at runtime by design so no need to be concurrent
Review Comment:
well it already does not work if done at runtime in a not safe context so I wouldn't be sorry and ultimately if you enable it at runtime how do you remove it today?
will fix the since
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Drop reflection from ExtraFieldUtils [commons-compress]
Posted by "rmannibucau (via GitHub)" <gi...@apache.org>.
rmannibucau commented on code in PR #480:
URL: https://github.com/apache/commons-compress/pull/480#discussion_r1485215496
##########
src/main/java/org/apache/commons/compress/archivers/zip/ExtraFieldUtils.java:
##########
@@ -113,24 +116,24 @@ public ZipExtraField onUnparseableExtraField(final byte[] data, final int off, f
/**
* Static registry of known extra fields.
*/
- private static final Map<ZipShort, Class<?>> IMPLEMENTATIONS;
+ private static final Map<ZipShort, Supplier<ZipExtraField>> IMPLEMENTATIONS;
static {
- IMPLEMENTATIONS = new ConcurrentHashMap<>();
- register(AsiExtraField.class);
- register(X5455_ExtendedTimestamp.class);
- register(X7875_NewUnix.class);
- register(JarMarker.class);
- register(UnicodePathExtraField.class);
- register(UnicodeCommentExtraField.class);
- register(Zip64ExtendedInformationExtraField.class);
- register(X000A_NTFS.class);
- register(X0014_X509Certificates.class);
- register(X0015_CertificateIdForFile.class);
- register(X0016_CertificateIdForCentralDirectory.class);
- register(X0017_StrongEncryptionHeader.class);
- register(X0019_EncryptionRecipientCertificateList.class);
- register(ResourceAlignmentExtraField.class);
+ IMPLEMENTATIONS = new HashMap<>(); // it can't be used at runtime by design so no need to be concurrent
Review Comment:
well it already does not work if done at runtime in a not safe context so I wouldn't be sorry and ultimately if you enable it at runtime how do you remove it today?
will fix the since
edit: updated
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Drop reflection from ExtraFieldUtils [commons-compress]
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #480:
URL: https://github.com/apache/commons-compress/pull/480#issuecomment-1936518750
## [Codecov](https://app.codecov.io/gh/apache/commons-compress/pull/480?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
Attention: `16 lines` in your changes are missing coverage. Please review.
> Comparison is base [(`1887b3d`)](https://app.codecov.io/gh/apache/commons-compress/commit/1887b3da14a9fdb5a29de3176de7b19d4885d9b3?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) 80.76% compared to head [(`f70746a`)](https://app.codecov.io/gh/apache/commons-compress/pull/480?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) 80.69%.
> Report is 22 commits behind head on master.
| [Files](https://app.codecov.io/gh/apache/commons-compress/pull/480?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Patch % | Lines |
|---|---|---|
| [...ommons/compress/archivers/zip/ExtraFieldUtils.java](https://app.codecov.io/gh/apache/commons-compress/pull/480?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2NvbW1vbnMvY29tcHJlc3MvYXJjaGl2ZXJzL3ppcC9FeHRyYUZpZWxkVXRpbHMuamF2YQ==) | 78.12% | [7 Missing :warning: ](https://app.codecov.io/gh/apache/commons-compress/pull/480?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [.../commons/compress/archivers/sevenz/SevenZFile.java](https://app.codecov.io/gh/apache/commons-compress/pull/480?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2NvbW1vbnMvY29tcHJlc3MvYXJjaGl2ZXJzL3NldmVuei9TZXZlblpGaWxlLmphdmE=) | 88.23% | [2 Missing and 2 partials :warning: ](https://app.codecov.io/gh/apache/commons-compress/pull/480?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [...compress/archivers/zip/X0014\_X509Certificates.java](https://app.codecov.io/gh/apache/commons-compress/pull/480?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2NvbW1vbnMvY29tcHJlc3MvYXJjaGl2ZXJzL3ppcC9YMDAxNF9YNTA5Q2VydGlmaWNhdGVzLmphdmE=) | 50.00% | [1 Missing :warning: ](https://app.codecov.io/gh/apache/commons-compress/pull/480?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [...ress/archivers/zip/X0015\_CertificateIdForFile.java](https://app.codecov.io/gh/apache/commons-compress/pull/480?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2NvbW1vbnMvY29tcHJlc3MvYXJjaGl2ZXJzL3ppcC9YMDAxNV9DZXJ0aWZpY2F0ZUlkRm9yRmlsZS5qYXZh) | 50.00% | [1 Missing :warning: ](https://app.codecov.io/gh/apache/commons-compress/pull/480?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [...rs/zip/X0016\_CertificateIdForCentralDirectory.java](https://app.codecov.io/gh/apache/commons-compress/pull/480?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2NvbW1vbnMvY29tcHJlc3MvYXJjaGl2ZXJzL3ppcC9YMDAxNl9DZXJ0aWZpY2F0ZUlkRm9yQ2VudHJhbERpcmVjdG9yeS5qYXZh) | 50.00% | [1 Missing :warning: ](https://app.codecov.io/gh/apache/commons-compress/pull/480?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [...ss/archivers/zip/X0017\_StrongEncryptionHeader.java](https://app.codecov.io/gh/apache/commons-compress/pull/480?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2NvbW1vbnMvY29tcHJlc3MvYXJjaGl2ZXJzL3ppcC9YMDAxN19TdHJvbmdFbmNyeXB0aW9uSGVhZGVyLmphdmE=) | 50.00% | [1 Missing :warning: ](https://app.codecov.io/gh/apache/commons-compress/pull/480?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [.../zip/X0019\_EncryptionRecipientCertificateList.java](https://app.codecov.io/gh/apache/commons-compress/pull/480?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2NvbW1vbnMvY29tcHJlc3MvYXJjaGl2ZXJzL3ppcC9YMDAxOV9FbmNyeXB0aW9uUmVjaXBpZW50Q2VydGlmaWNhdGVMaXN0LmphdmE=) | 50.00% | [1 Missing :warning: ](https://app.codecov.io/gh/apache/commons-compress/pull/480?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## master #480 +/- ##
============================================
- Coverage 80.76% 80.69% -0.08%
+ Complexity 6801 6799 -2
============================================
Files 348 348
Lines 24945 24978 +33
Branches 4029 4030 +1
============================================
+ Hits 20148 20157 +9
- Misses 3269 3293 +24
Partials 1528 1528
```
</details>
[:umbrella: View full report in Codecov by Sentry](https://app.codecov.io/gh/apache/commons-compress/pull/480?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
:loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Drop reflection from ExtraFieldUtils [commons-compress]
Posted by "rmannibucau (via GitHub)" <gi...@apache.org>.
rmannibucau commented on PR #480:
URL: https://github.com/apache/commons-compress/pull/480#issuecomment-1937101399
@garydgregory very good catch! fixed now normally. On the builder point I had the same thought and I think it is maybe worth to not expose all constructors (package scope?) and just use a single one for next release, the builder will make the api more verbose with a very low gain since most constructors are just about calling setters - and extra fields are exactly there for example.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Drop reflection from ExtraFieldUtils [commons-compress]
Posted by "garydgregory (via GitHub)" <gi...@apache.org>.
garydgregory commented on code in PR #480:
URL: https://github.com/apache/commons-compress/pull/480#discussion_r1485214180
##########
src/main/java/org/apache/commons/compress/archivers/zip/ExtraFieldUtils.java:
##########
@@ -113,24 +116,24 @@ public ZipExtraField onUnparseableExtraField(final byte[] data, final int off, f
/**
* Static registry of known extra fields.
*/
- private static final Map<ZipShort, Class<?>> IMPLEMENTATIONS;
+ private static final Map<ZipShort, Supplier<ZipExtraField>> IMPLEMENTATIONS;
static {
- IMPLEMENTATIONS = new ConcurrentHashMap<>();
- register(AsiExtraField.class);
- register(X5455_ExtendedTimestamp.class);
- register(X7875_NewUnix.class);
- register(JarMarker.class);
- register(UnicodePathExtraField.class);
- register(UnicodeCommentExtraField.class);
- register(Zip64ExtendedInformationExtraField.class);
- register(X000A_NTFS.class);
- register(X0014_X509Certificates.class);
- register(X0015_CertificateIdForFile.class);
- register(X0016_CertificateIdForCentralDirectory.class);
- register(X0017_StrongEncryptionHeader.class);
- register(X0019_EncryptionRecipientCertificateList.class);
- register(ResourceAlignmentExtraField.class);
+ IMPLEMENTATIONS = new HashMap<>(); // it can't be used at runtime by design so no need to be concurrent
Review Comment:
The method `register(Class)` is a public API, so anyone can call it at any time, I don't see the need to change the Map implementation, better safe than sorry ;-)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Drop reflection from ExtraFieldUtils [commons-compress]
Posted by "garydgregory (via GitHub)" <gi...@apache.org>.
garydgregory commented on code in PR #480:
URL: https://github.com/apache/commons-compress/pull/480#discussion_r1484812539
##########
src/main/java/org/apache/commons/compress/archivers/zip/ExtraFieldUtils.java:
##########
@@ -379,11 +382,28 @@ public ZipExtraField onUnparseableExtraField(final byte[] data, final int off, f
* </p>
*
* @param c the class to register
+ *
+ * todo: "at_deprecated this registration is global and shared by all consumers.", find a way to make it specific.
Review Comment:
Hi @rmannibucau
Thank you for your PR. Was this meant to be a draft PR? I don't think we should leave TODO's behind.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Drop reflection from ExtraFieldUtils static initialization [commons-compress]
Posted by "garydgregory (via GitHub)" <gi...@apache.org>.
garydgregory merged PR #480:
URL: https://github.com/apache/commons-compress/pull/480
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Drop reflection from ExtraFieldUtils [commons-compress]
Posted by "garydgregory (via GitHub)" <gi...@apache.org>.
garydgregory commented on PR #480:
URL: https://github.com/apache/commons-compress/pull/480#issuecomment-1937102315
> @garydgregory very good catch! fixed now normally. On the builder point I had the same thought and I think it is maybe worth to not expose all constructors (package scope?) and just use a single one for next release, the builder will make the api more verbose with a very low gain since most constructors are just about calling setters - and extra fields are exactly there for example.
Hi @rmannibucau
- I think that for this PR, the constructor changes should be as minimal and visible as you can make them, so yes, package-private or private is the way to go.
- Once this PR is done, I can experiment with a Builder.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Drop reflection from ExtraFieldUtils static initialization [commons-compress]
Posted by "garydgregory (via GitHub)" <gi...@apache.org>.
garydgregory commented on PR #480:
URL: https://github.com/apache/commons-compress/pull/480#issuecomment-1937141258
@rmannibucau
Merged!
Thank you for your work 😄
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Drop reflection from ExtraFieldUtils [commons-compress]
Posted by "garydgregory (via GitHub)" <gi...@apache.org>.
garydgregory commented on code in PR #480:
URL: https://github.com/apache/commons-compress/pull/480#discussion_r1485144279
##########
src/main/java/org/apache/commons/compress/archivers/zip/ExtraFieldUtils.java:
##########
@@ -379,11 +382,28 @@ public ZipExtraField onUnparseableExtraField(final byte[] data, final int off, f
* </p>
*
* @param c the class to register
+ *
+ * todo: "at_deprecated this registration is global and shared by all consumers.", find a way to make it specific.
Review Comment:
IMO: Either:
- Remove the TODO, or
- Deprecate the method and make its replacement public.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Drop reflection from ExtraFieldUtils [commons-compress]
Posted by "rmannibucau (via GitHub)" <gi...@apache.org>.
rmannibucau commented on code in PR #480:
URL: https://github.com/apache/commons-compress/pull/480#discussion_r1485205287
##########
src/main/java/org/apache/commons/compress/archivers/zip/ExtraFieldUtils.java:
##########
@@ -379,11 +382,28 @@ public ZipExtraField onUnparseableExtraField(final byte[] data, final int off, f
* </p>
*
* @param c the class to register
+ *
+ * todo: "at_deprecated this registration is global and shared by all consumers.", find a way to make it specific.
Review Comment:
pushed fixing it
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org