[compress] Need Feedback for COMPRESS-479

classic Classic list List threaded Threaded
7 messages Options
Reply | Threaded
Open this post in threaded view
|

[compress] Need Feedback for COMPRESS-479

Stefan Bodewig
Hi

https://issues.apache.org/jira/browse/COMPRESS-479 highlights a problem
where our zip reading code may reject an archive because it uses extra
fields that we only partially understand. In this case the archive is
not a real one, but it is easy to envision extra fields in the wild that
use more advanced versions than we currently support.

For most of our users the ZIP extra fields will be noise and they really
are only interested in the actual content of the archives.

Therefore I have decided to treat any such "not quite as we expect"
extra field the same way as any "we have no idea how this extra field
looks internally" field - i.e. just store it but don't try to parse
it.

In addition I have provided an API inside of ZipArchiveEntry that allows
users to specify in detail how strict they want the extra field parsing
to be.

Does this make sense to you? Would you recommend taking a different
approach? And it if makes sense, are the enum names I've chosen in
https://github.com/apache/commons-compress/blob/2bf678bbc6c6002569559b90ea29a031f035f067/src/main/java/org/apache/commons/compress/archivers/zip/ZipArchiveEntry.java#L1117
understandable?

Thanks

        Stefan

---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

Re: [compress] Need Feedback for COMPRESS-479

Matt Sicker
The enum makes sense. Are there any feasible ways to, say, configure
some sort of handler class that can implement logic around unknown
fields? Sort of an extensibility model there, but hopefully without
having to extend ZipInputStream or anything like that.

On Tue, 13 Aug 2019 at 14:01, Stefan Bodewig <[hidden email]> wrote:

>
> Hi
>
> https://issues.apache.org/jira/browse/COMPRESS-479 highlights a problem
> where our zip reading code may reject an archive because it uses extra
> fields that we only partially understand. In this case the archive is
> not a real one, but it is easy to envision extra fields in the wild that
> use more advanced versions than we currently support.
>
> For most of our users the ZIP extra fields will be noise and they really
> are only interested in the actual content of the archives.
>
> Therefore I have decided to treat any such "not quite as we expect"
> extra field the same way as any "we have no idea how this extra field
> looks internally" field - i.e. just store it but don't try to parse
> it.
>
> In addition I have provided an API inside of ZipArchiveEntry that allows
> users to specify in detail how strict they want the extra field parsing
> to be.
>
> Does this make sense to you? Would you recommend taking a different
> approach? And it if makes sense, are the enum names I've chosen in
> https://github.com/apache/commons-compress/blob/2bf678bbc6c6002569559b90ea29a031f035f067/src/main/java/org/apache/commons/compress/archivers/zip/ZipArchiveEntry.java#L1117
> understandable?
>
> Thanks
>
>         Stefan
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: [hidden email]
> For additional commands, e-mail: [hidden email]
>


--
Matt Sicker <[hidden email]>

---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

Re: [compress] Need Feedback for COMPRESS-479

garydgregory
In reply to this post by Stefan Bodewig
Hi Stefan,

I like having predefined behaviors in the enum but it does not seem to
allow for extension. I am wondering if we would be better served by using
an interface invoked by a callback mechanism where predefined
implementations exist for the enum values you now have. I would see in
addition, a SystemOut impl that printfs these extra fields we do not handle.

Gary

On Tue, Aug 13, 2019 at 3:01 PM Stefan Bodewig <[hidden email]> wrote:

> Hi
>
> https://issues.apache.org/jira/browse/COMPRESS-479 highlights a problem
> where our zip reading code may reject an archive because it uses extra
> fields that we only partially understand. In this case the archive is
> not a real one, but it is easy to envision extra fields in the wild that
> use more advanced versions than we currently support.
>
> For most of our users the ZIP extra fields will be noise and they really
> are only interested in the actual content of the archives.
>
> Therefore I have decided to treat any such "not quite as we expect"
> extra field the same way as any "we have no idea how this extra field
> looks internally" field - i.e. just store it but don't try to parse
> it.
>
> In addition I have provided an API inside of ZipArchiveEntry that allows
> users to specify in detail how strict they want the extra field parsing
> to be.
>
> Does this make sense to you? Would you recommend taking a different
> approach? And it if makes sense, are the enum names I've chosen in
>
> https://github.com/apache/commons-compress/blob/2bf678bbc6c6002569559b90ea29a031f035f067/src/main/java/org/apache/commons/compress/archivers/zip/ZipArchiveEntry.java#L1117
> understandable?
>
> Thanks
>
>         Stefan
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: [hidden email]
> For additional commands, e-mail: [hidden email]
>
>
Reply | Threaded
Open this post in threaded view
|

Re: [compress] Need Feedback for COMPRESS-479

Stefan Bodewig
In reply to this post by Matt Sicker
On 2019-08-13, Matt Sicker wrote:

> The enum makes sense. Are there any feasible ways to, say, configure
> some sort of handler class that can implement logic around unknown
> fields?

Not really. The only extension point here currently is plugging in your
own implementations of ZipExtraField via the static
ExtraFieldUtils.register - which could use some ServiceLoader magic one
day :-)

IIUC you and Gary are both saying the same thing. The enum values are
sensible defaults but it would be good to provide a way to do the same
things with custom code (callbacks or interface implementations).

It should be possible to split ExtraFieldParsingMode into a strategy
pattern interface implemented by the enum providing default
implementations. This may also reduce some other implementation quirks
(I'm not too happy with the current exception handler inside
mergeExtraFields).

Thanks!

        Stefan

---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

Re: [compress] Need Feedback for COMPRESS-479

Matt Sicker
Yes, I think you understand us. A strategy pattern with default sensible
strategies to choose.

On Wed, Aug 14, 2019 at 06:08, Stefan Bodewig <[hidden email]> wrote:

> On 2019-08-13, Matt Sicker wrote:
>
> > The enum makes sense. Are there any feasible ways to, say, configure
> > some sort of handler class that can implement logic around unknown
> > fields?
>
> Not really. The only extension point here currently is plugging in your
> own implementations of ZipExtraField via the static
> ExtraFieldUtils.register - which could use some ServiceLoader magic one
> day :-)
>
> IIUC you and Gary are both saying the same thing. The enum values are
> sensible defaults but it would be good to provide a way to do the same
> things with custom code (callbacks or interface implementations).
>
> It should be possible to split ExtraFieldParsingMode into a strategy
> pattern interface implemented by the enum providing default
> implementations. This may also reduce some other implementation quirks
> (I'm not too happy with the current exception handler inside
> mergeExtraFields).
>
> Thanks!
>
>         Stefan
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: [hidden email]
> For additional commands, e-mail: [hidden email]
>
> --
Matt Sicker <[hidden email]>
Reply | Threaded
Open this post in threaded view
|

Re: [compress] Need Feedback for COMPRESS-479

garydgregory
We all understood each other in a brief email thread, remarkable! :-)

Gary

On Wed, Aug 14, 2019 at 9:00 AM Matt Sicker <[hidden email]> wrote:

> Yes, I think you understand us. A strategy pattern with default sensible
> strategies to choose.
>
> On Wed, Aug 14, 2019 at 06:08, Stefan Bodewig <[hidden email]> wrote:
>
> > On 2019-08-13, Matt Sicker wrote:
> >
> > > The enum makes sense. Are there any feasible ways to, say, configure
> > > some sort of handler class that can implement logic around unknown
> > > fields?
> >
> > Not really. The only extension point here currently is plugging in your
> > own implementations of ZipExtraField via the static
> > ExtraFieldUtils.register - which could use some ServiceLoader magic one
> > day :-)
> >
> > IIUC you and Gary are both saying the same thing. The enum values are
> > sensible defaults but it would be good to provide a way to do the same
> > things with custom code (callbacks or interface implementations).
> >
> > It should be possible to split ExtraFieldParsingMode into a strategy
> > pattern interface implemented by the enum providing default
> > implementations. This may also reduce some other implementation quirks
> > (I'm not too happy with the current exception handler inside
> > mergeExtraFields).
> >
> > Thanks!
> >
> >         Stefan
> >
> > ---------------------------------------------------------------------
> > To unsubscribe, e-mail: [hidden email]
> > For additional commands, e-mail: [hidden email]
> >
> > --
> Matt Sicker <[hidden email]>
>
Reply | Threaded
Open this post in threaded view
|

Re: [compress] Need Feedback for COMPRESS-479

Stefan Bodewig
In reply to this post by Matt Sicker
On 2019-08-14, Matt Sicker wrote:

> Yes, I think you understand us. A strategy pattern with default sensible
> strategies to choose.

Done now.

I'm going to tweak the implementation a little more but the API of
ExtraFieldParsingBehavior seems to work quite well.

Thanks

        Stefan

---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]