refactor: mark ZipFlags as non-exhaustive and add test for HasZipMetadata#777
Conversation
There was a problem hiding this comment.
This refactoring correctly applies #[non_exhaustive] attributes and adjusts visibility modifiers without introducing any functional defects. The changes improve API stability by preventing external code from exhaustively matching on these enums/structs or directly accessing internal fields.
You can now have the agent implement changes and create commits directly on your pull request's source branch. Simply comment with /q followed by your request in natural language to ask the agent to make changes.
There was a problem hiding this comment.
Code Review
This pull request introduces the #[non_exhaustive] attribute to several structures and enums, including ZipFile, ZipFileSeek, and ZipFlags, to improve future API compatibility. It also attempts to encapsulate ZipFileData by changing the visibility of the struct and its fields to pub(crate). However, a critical issue was identified where making ZipFileData private to the crate causes compilation errors because it is still exposed through the public HasZipMetadata trait.
| pub(crate) struct ZipFileData { | ||
| /// Compatibility of the file attribute information | ||
| pub system: System, | ||
| pub(crate) system: System, | ||
| /// Specification version | ||
| pub version_made_by: u8, | ||
| pub(crate) version_made_by: u8, | ||
| /// ZIP flags | ||
| pub flags: u16, | ||
| pub(crate) flags: u16, | ||
| /// True if the file is encrypted. | ||
| pub encrypted: bool, | ||
| pub(crate) encrypted: bool, | ||
| /// True if `file_name` and `file_comment` are UTF8 | ||
| pub is_utf8: bool, | ||
| pub(crate) is_utf8: bool, | ||
| /// True if the file uses a data-descriptor section | ||
| pub using_data_descriptor: bool, | ||
| pub(crate) using_data_descriptor: bool, | ||
| /// Compression method used to store the file | ||
| pub compression_method: crate::compression::CompressionMethod, | ||
| pub(crate) compression_method: crate::compression::CompressionMethod, | ||
| /// Compression level to store the file | ||
| pub compression_level: Option<i64>, | ||
| pub(crate) compression_level: Option<i64>, | ||
| /// Last modified time. This will only have a 2 second precision. | ||
| pub last_modified_time: Option<DateTime>, | ||
| pub(crate) last_modified_time: Option<DateTime>, | ||
| /// CRC32 checksum | ||
| pub crc32: u32, | ||
| pub(crate) crc32: u32, | ||
| /// Size of the file in the ZIP | ||
| pub compressed_size: u64, | ||
| pub(crate) compressed_size: u64, | ||
| /// Size of the file when extracted | ||
| pub uncompressed_size: u64, | ||
| pub(crate) uncompressed_size: u64, | ||
| /// Name of the file | ||
| pub file_name: Box<str>, | ||
| pub(crate) file_name: Box<str>, | ||
| /// Raw file name. To be used when `file_name` was incorrectly decoded. | ||
| pub file_name_raw: Box<[u8]>, | ||
| pub(crate) file_name_raw: Box<[u8]>, | ||
| /// Extra field usually used for storage expansion | ||
| pub extra_field: Option<Arc<[u8]>>, | ||
| pub(crate) extra_field: Option<Arc<[u8]>>, | ||
| /// Extra field only written to central directory | ||
| pub central_extra_field: Option<Arc<[u8]>>, | ||
| pub(crate) central_extra_field: Option<Arc<[u8]>>, | ||
| /// File comment | ||
| pub file_comment: Box<str>, | ||
| pub(crate) file_comment: Box<str>, | ||
| /// Specifies where the local header of the file starts | ||
| pub header_start: u64, | ||
| pub(crate) header_start: u64, | ||
| /// Specifies where the extra data of the file starts | ||
| pub extra_data_start: Option<u64>, | ||
| pub(crate) extra_data_start: Option<u64>, | ||
| /// Specifies where the central header of the file starts | ||
| /// | ||
| /// Note that when this is not known, it is set to 0 | ||
| pub central_header_start: u64, | ||
| pub(crate) central_header_start: u64, | ||
| /// Specifies where the compressed data of the file starts | ||
| pub data_start: OnceLock<u64>, | ||
| pub(crate) data_start: OnceLock<u64>, | ||
| /// External file attributes | ||
| pub external_attributes: u32, | ||
| pub(crate) external_attributes: u32, | ||
| /// Reserve local ZIP64 extra field | ||
| pub large_file: bool, | ||
| pub(crate) large_file: bool, | ||
| /// AES mode if applicable | ||
| pub aes_mode: Option<(AesMode, AesVendorVersion, CompressionMethod)>, | ||
| pub(crate) aes_mode: Option<(AesMode, AesVendorVersion, CompressionMethod)>, | ||
| /// Specifies where in the extra data the AES metadata starts | ||
| pub aes_extra_data_start: u64, | ||
| pub(crate) aes_extra_data_start: u64, | ||
|
|
||
| /// extra fields, see <https://libzip.org/specifications/extrafld.txt> | ||
| pub extra_fields: Vec<ExtraField>, | ||
| pub(crate) extra_fields: Vec<ExtraField>, |
There was a problem hiding this comment.
Changing ZipFileData to pub(crate) results in a compilation error because it is currently exposed through the public trait HasZipMetadata. While our guidelines state that we should avoid exposing internal data structures in public APIs, this change is a breaking one and causes build failures. Please refactor the public interfaces to use primitive types or opaque handles instead of exposing the internal struct, and ensure all dependent implementations are updated accordingly to avoid compilation errors.
References
- Avoid exposing internal data structures in public APIs. Instead, accept primitive types (e.g., &[u8]) and perform validation and construction internally.
- When changing the type of a field, ensure all implementations that use this field are updated accordingly to avoid compilation errors.
There was a problem hiding this comment.
pub(crate)HasZipMetadata
Pr0methean
left a comment
There was a problem hiding this comment.
It's unclear to me why any of these structs/enums except ZipFlags should be #[non_exhaustive].
Security Policy Alert: Secret Policy ViolationThis workflow run has been blocked by StepSecurity's secrets policy because it accesses secrets and the workflow file differs from the default branch. To approve this workflow, please add the Note: The label must be added by someone other than the PR author (Its-Just-Nans) or automation bots to ensure proper security review. After the label is added, you can re-run the blocked workflow to proceed. This workflow will be automatically approved once merged into the default branch. For more information, see StepSecurity's Secret Exfiltration Policy documentation. |
HasZipMetadataZipFlags as non-exhaustive and add test for HasZipMetadata
No changes, just refactoring for better readability (I thought that
ZipFileDatawas public but it's not !)