Re: [PR] Drop reflection from ExtraFieldUtils [commons-compress]
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-unsubscr...@commons.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] Drop reflection from ExtraFieldUtils [commons-compress]
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-unsubscr...@commons.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] Drop reflection from ExtraFieldUtils [commons-compress]
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> IMPLEMENTATIONS;
+private static final Map>
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-unsubscr...@commons.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] Drop reflection from ExtraFieldUtils [commons-compress]
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> IMPLEMENTATIONS;
+private static final Map>
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-unsubscr...@commons.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] Drop reflection from ExtraFieldUtils [commons-compress]
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> IMPLEMENTATIONS;
+private static final Map>
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-unsubscr...@commons.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] Drop reflection from ExtraFieldUtils [commons-compress]
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 * * * @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-unsubscr...@commons.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] Drop reflection from ExtraFieldUtils [commons-compress]
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 * * * @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 * * * @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-unsubscr...@commons.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] Drop reflection from ExtraFieldUtils [commons-compress]
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 * * * @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-unsubscr...@commons.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] Drop reflection from ExtraFieldUtils [commons-compress]
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 * * * @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-unsubscr...@commons.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] Drop reflection from ExtraFieldUtils [commons-compress]
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 * * * @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-unsubscr...@commons.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
