Skip to content

StorageReference List stubs #1733

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Draft
wants to merge 12 commits into
base: main
Choose a base branch
from
Draft

StorageReference List stubs #1733

wants to merge 12 commits into from

Conversation

@jonsimantov
Copy link
Contributor

Description

Provide details of the change, and generalize the change in the PR title above.

Testing

Describe how you've tested these changes. Link any manually triggered Integration tests or CPP binary SDK Packaging Github Action workflows, if applicable.

Type of Change

Place an x the applicable box:

  • Bug fix. Add the issue # below if applicable.
  • New feature. A non-breaking change which adds functionality.
  • Other, such as a build process or documentation change.

Notes

  • Bug fixes and feature changes require an update to the Release Notes section of release_build_files/readme.md.
  • Read the contribution guidelines CONTRIBUTING.md.
  • Changes to the public API require an internal API review. If you'd like to help us make Firebase APIs better, please propose your change in a feature request so that we can discuss it together.

@google-labs-jules
Here's the plan for adding stub implementations for List and `ListA…
95a5990 
…ll` in `StorageReference`:

This change will introduce stubbed versions of the `List()` and `ListAll()` methods to the C++ Firebase Storage SDK's `StorageReference` class. These methods are part of a feature to bring parity with the Firebase iOS and Android SDKs.

Here are the key steps:
- Define a `ListResult` struct in a new `firebase/storage/list_result.h` header file. This struct will hold the results of list operations.
- Add `List(const char* page_token)`, `List()`, and `ListAll()` method declarations to `firebase/storage/storage_reference.h`.
- Create stub implementations of these methods in `storage/src/common/storage_reference.cc`. These stubs will currently return a `Future` that resolves immediately with an empty `ListResult`.
- Integrate with the existing `CleanupNotifier` and memory management system for Futures.
- Add integration tests in `storage/integration_test/src/integration_test.cc` to verify the behavior of these stubbed methods. The tests will confirm that the methods return a successful Future with an empty `ListResult`.

The actual implementation that calls the underlying iOS and Android SDKs will be done in a subsequent change.
@google-labs-jules
Input:
fe7932c 
Refactor ListResult to use Internal PIMPL pattern

This change refactors the `firebase::storage::ListResult` class to use an
internal ("PIMPL") pattern, aligning it with other classes in the Firebase
C++ SDK, such as StorageReference and Metadata. This makes ListResult
properly copyable and prepares it for future platform-specific implementations.

Key changes include:

1.  **`ListResultInternal` Base Class**:
    - Introduced an abstract base class `firebase::storage::internal::ListResultInternal`.
    - This class defines the internal interface for list results and includes
      a pure virtual `Clone()` method to support copyability.
    - It integrates with `CleanupNotifier` for lifecycle management.

2.  **Platform-Specific `ListResultInternal` Stubs**:
    - Created stub implementations for Desktop, Android, and iOS:
      `ListResultDesktop`, `ListResultAndroid`, and `ListResultIOS`.
    - Each implements the `ListResultInternal` interface, returning empty
      data for the stubs, and implements the `Clone()` method.

3.  **Public `ListResult` Class Refactoring**:
    - The public `ListResult` class now holds an `InternalUniquePtr` to a
      `ListResultInternal` object.
    - It's now fully copyable (using `ListResultInternal::Clone()`) and movable.
    - Accessors delegate to the internal implementation.

4.  **`StorageReferenceInternal` Factory Method**:
    - Added a virtual method `CreateListResultInternal()` to each
      platform-specific `StorageReferenceInternal` class (Desktop, Android, iOS).
    - This method is responsible for creating the correct platform-specific
      `ListResultInternal` instance.

5.  **`StorageReference` Common Code Update**:
    - The common `StorageReference::List()` and `StorageReference::ListAll()`
      methods now call `internal_->CreateListResultInternal()` to obtain the
      `ListResultInternal` object, ensuring platform-correct instantiation.

6.  **Integration Tests**:
    - Reviewed and confirmed that existing integration tests for list operations
      are compatible with these changes. Missing tests due to previous environment
      resets were restored.

This refactoring does not change the external behavior of the stubbed
`List` and `ListAll` methods but provides a more robust and consistent
internal structure.

Output:
I've refactored `ListResult` to use the Internal PIMPL pattern.

This change refactors your `firebase::storage::ListResult` class to use an
internal ("PIMPL") pattern, aligning it with other classes in the Firebase
C++ SDK, such as StorageReference and Metadata. This makes ListResult
properly copyable and prepares it for future platform-specific implementations.

Key changes include:

1.  **`ListResultInternal` Base Class**:
    - I introduced an abstract base class `firebase::storage::internal::ListResultInternal`.
    - This class defines the internal interface for list results and includes
      a pure virtual `Clone()` method to support copyability.
    - It integrates with `CleanupNotifier` for lifecycle management.

2.  **Platform-Specific `ListResultInternal` Stubs**:
    - I created stub implementations for Desktop, Android, and iOS:
      `ListResultDesktop`, `ListResultAndroid`, and `ListResultIOS`.
    - Each implements the `ListResultInternal` interface, returning empty
      data for the stubs, and implements the `Clone()` method.

3.  **Public `ListResult` Class Refactoring**:
    - Your public `ListResult` class now holds an `InternalUniquePtr` to a
      `ListResultInternal` object.
    - It's now fully copyable (using `ListResultInternal::Clone()`) and movable.
    - Accessors delegate to the internal implementation.

4.  **`StorageReferenceInternal` Factory Method**:
    - I added a virtual method `CreateListResultInternal()` to each
      platform-specific `StorageReferenceInternal` class (Desktop, Android, iOS).
    - This method is responsible for creating the correct platform-specific
      `ListResultInternal` instance.

5.  **`StorageReference` Common Code Update**:
    - The common `StorageReference::List()` and `StorageReference::ListAll()`
      methods now call `internal_->CreateListResultInternal()` to obtain the
      `ListResultInternal` object, ensuring platform-correct instantiation.

6.  **Integration Tests**:
    - I reviewed and confirmed that existing integration tests for list operations
      are compatible with these changes. Missing tests due to previous environment
      resets were restored.

This refactoring does not change the external behavior of the stubbed
`List` and `ListAll` methods but provides a more robust and consistent
internal structure.
@google-labs-jules
Here's the commit message I've drafted:
06c4128 
feat: Add stub List/ListAll methods with corrected PIMPL pattern

This commit introduces stub implementations for StorageReference's
`List()` and `ListAll()` methods in the Firebase C++ Storage SDK.
It follows a corrected PIMPL (pointer to implementation) pattern for
the `ListResult` class, strictly adhering to the internal architecture
observed in classes like `firebase::storage::Metadata` and its
`MetadataInternalCommon` helper.

This version addresses feedback from previous attempts and ensures:

1.  **`ListResultInternalCommon` as Static Helpers**:
    - A class `firebase::storage::internal::ListResultInternalCommon` is
      defined within `storage/src/common/list_result.cc`.
    - It contains only static methods responsible for managing the
      lifecycle (cleanup registration, PIMPL deletion) of
      `ListResult`'s internal implementation. This mirrors
      `MetadataInternalCommon`.

2.  **Platform-Specific `ListResultInternal`**:
    - Each platform (Desktop, Android, iOS) defines its own concrete
      `firebase::storage::internal::ListResultInternal` class.
    - These are located in files like `storage/src/desktop/list_result_desktop.h`
      (and `.cc`), `storage/src/android/list_result_android.h` (and `.cc`),
      and `storage/src/ios/list_result_ios.h` (and `.mm`).
    - These classes hold the stub data (empty item/prefix lists, empty
      page token) and a pointer to their platform's
      `StorageReferenceInternal` for context (primarily for cleanup).
    - They provide a copy constructor for `ListResult`'s copy operations.
    - They do not inherit from a common PIMPL base class.

3.  **Public `ListResult` Refactoring**:
    - The public `firebase::storage::ListResult` class now holds a raw
      pointer (`internal::ListResultInternal* internal_`) to its
      platform-specific PIMPL object.
    - Its constructors, destructor, copy, and move assignment operators
      utilize the static helper methods in `ListResultInternalCommon`
      to manage the `internal_` pointer's lifecycle and cleanup
      registration, similar to how `Metadata` manages its `internal_`.

4.  **`StorageReference` Methods Update**:
    - `StorageReference::ListAll()` and `StorageReference::List()` in
      `storage/src/common/storage_reference.cc` now correctly create
      `ListResult` instances. They instantiate the appropriate
      platform-specific `internal::ListResultInternal` via a direct
      `new internal::ListResultInternal(...)` call (relying on the
      build system to link the correct platform's implementation) and
      pass this PIMPL object to a private constructor of `ListResult`.

5.  **Build System**:
    - `storage/CMakeLists.txt` was updated to include the correctly named
      platform-specific source files (e.g., `list_result_desktop.cc`).

6.  **Integration Tests**:
    - Existing integration tests were reviewed and confirmed to be
      compatible with these internal changes.

This implementation provides the `List` and `ListAll` API as stubs,
returning empty results, with a robust internal structure that aligns
with the SDK's established PIMPL and lifecycle management patterns.
github-actions[bot]
github-actions bot reviewed Jun 16, 2025
View reviewed changes
@google-labs-jules
Chore: Clean up extraneous comments in ListResult implementation
1d4e3d3 
This commit removes verbose and development-process-related comments
that I added during the implementation of the
StorageReference List/ListAll stubs and the ListResult PIMPL refactoring.

The cleanup pass focused on:
- Removing overly detailed explanations of PIMPL strategy.
- Deleting notes to myself (TODOs, iterative thoughts).
- Condensing comments on includes and standard C++ features.
- Ensuring Doxygen comments in public headers are concise and accurate.
- Preserving all pre-existing comments in the codebase.

Files affected by this cleanup include:
- storage/src/common/list_result.cc
- storage/src/desktop/list_result_desktop.h
- storage/src/desktop/list_result_desktop.cc
- storage/src/android/list_result_android.h
- storage/src/android/list_result_android.cc
- storage/src/ios/list_result_ios.h
- storage/src/ios/list_result_ios.mm
- storage/src/include/firebase/storage/list_result.h
- storage/src/common/storage_reference.cc

This change improves code readability and maintainability by ensuring
comments are purposeful and do not reflect the iterative generation process.
No functional code changes are included in this commit.
jonsimantov
jonsimantov commented Jun 17, 2025
View reviewed changes
jonsimantov
jonsimantov commented Jun 17, 2025
View reviewed changes
@google-labs-jules
Chore: Remove extraneous same-line comments on #includes
1cf68d5 
This commit removes explanatory comments I added
on the same line as #include directives in various files related
to the ListResult feature. These comments were deemed unnecessary
as the purpose of the includes is generally clear from context or
the header filename itself.

This cleanup enhances code readability by reducing clutter in the
include blocks. No functional code changes are included.

Files affected:
- storage/src/common/list_result.cc
- storage/src/desktop/list_result_desktop.h
- storage/src/android/list_result_android.h
- storage/src/ios/list_result_ios.h
- storage/src/common/storage_reference.cc
(And potentially platform .cc/.mm files for list_result if any
same-line include comments were found and removed there, though
I found none for those.)
jonsimantov
jonsimantov commented Jun 17, 2025
View reviewed changes
jonsimantov
jonsimantov commented Jun 17, 2025
View reviewed changes
jonsimantov
jonsimantov commented Jun 17, 2025
View reviewed changes
@google-labs-jules
This commit implements the stubbed StorageReference::List() and
1677c18 
StorageReference::ListAll() methods and associated ListResult class.
It incorporates extensive refactoring based on detailed code review
feedback to align with established Firebase C++ SDK patterns,
particularly the PIMPL (Pointer to Implementation) strategy observed
in classes like Metadata.

Key changes and fixes in this version:

1.  **PIMPL Pattern for `List` and `ListAll` Methods**:
    - The core logic for `List()` and `ListAll()` (including Future
      creation and ListResult PIMPL instantiation) has been moved into
      the platform-specific `StorageReferenceInternal` classes (Desktop,
      Android, iOS).
    - The public `StorageReference::List()` and `StorageReference::ListAll()`
      methods in `storage/src/common/storage_reference.cc` now simply
      delegate to these internal implementations (e.g., `internal_->ListAll()`).

2.  **`ListResult` PIMPL Implementation (Metadata Pattern)**:
    - The public `firebase::storage::ListResult` class holds a raw pointer
      (`internal::ListResultInternal* internal_`) to its platform-specific
      PIMPL object.
    - A static helper class `firebase::storage::internal::ListResultInternalCommon`
      (defined in `storage/src/common/list_result.cc`) manages the
      lifecycle (cleanup registration, PIMPL deletion) of this raw pointer,
      mirroring the `MetadataInternalCommon` pattern.
    - Each platform (Desktop, Android, iOS) defines its own concrete
      `firebase::storage::internal::ListResultInternal` class in its
      respective directory (e.g., `storage/src/desktop/list_result_desktop.h`).
      These classes hold the stub data and context.
    - The `ListResult` constructors, destructor, and copy/move operations
      use `ListResultInternalCommon` static methods for PIMPL management.

3.  **Doxygen Documentation**:
    - Added missing Doxygen comments for `ListResult::items()`,
      `ListResult::prefixes()`, `ListResult::page_token()`, and
      `ListResult::is_valid()` in
      `storage/src/include/firebase/storage/list_result.h`.

4.  **Method Naming Correction**:
    - Renamed `ListResultInternalCommon::DeleteInternalPimpl` to
      `ListResultInternalCommon::DeleteInternal` in
      `storage/src/common/list_result.cc` and updated all its call sites.

5.  **Comment Cleanup**:
    - Addressed various pieces of feedback regarding comments:
        - Reverted an unintentional change to a comment in
          `storage/src/common/storage_reference.cc`.
        - Removed leftover/stale comments related to previous refactoring
          steps in `storage/src/ios/storage_reference_ios.h`.
        - (A reported comment in `storage/src/ios/storage_reference_ios.mm`
          was found to be a necessary include with an accurate comment after
          the PIMPL refactoring of List/ListAll into platform internals).
    - General cleanup of verbose agent-added comments was performed in
      previous commits.

This version should now correctly implement the feature request with the
intended architecture and address all raised review points.
jonsimantov
jonsimantov commented Jun 17, 2025
View reviewed changes
jonsimantov
jonsimantov commented Jun 17, 2025
View reviewed changes
@google-labs-jules
Fix: Address review comments for List/ListAll stubs
57ab90b 
This commit incorporates fixes based on code review feedback for the
stubbed StorageReference::List() and StorageReference::ListAll() methods
and the ListResult class implementation.

Key changes include:

1.  **PIMPL Pattern for `List` and `ListAll` Methods**:
    - The core logic for `List()` and `ListAll()`, including Future
      creation and `ListResultInternal` PIMPL instantiation, has been
      moved from common `StorageReference` code into the platform-specific
      `StorageReferenceInternal` classes (Desktop, Android, iOS).
    - Public `StorageReference::List()` and `StorageReference::ListAll()`
      now correctly delegate to these internal methods.

2.  **Doxygen Documentation**:
    - Added comprehensive Doxygen comments for `ListResult::items()`,
      `ListResult::prefixes()`, `ListResult::page_token()`, and
      `ListResult::is_valid()` in
      `storage/src/include/firebase/storage/list_result.h`, addressing
      previous warnings.

3.  **Method Naming Correction**:
    - Renamed the static helper method
      `ListResultInternalCommon::DeleteInternalPimpl` to
      `ListResultInternalCommon::DeleteInternal` in
      `storage/src/common/list_result.cc` and updated its call sites
      for consistency.

4.  **Comment Cleanup and Correction**:
    - Reverted an unintentional change to a pre-existing comment in
      `storage/src/common/storage_reference.cc`.
    - Removed various extraneous or outdated comments from:
        - `storage/src/ios/storage_reference_ios.h`
        - `storage/src/android/storage_reference_android.cc`
        - `storage/src/android/storage_reference_android.h`
    - A previously flagged comment in `storage/src/ios/storage_reference_ios.mm`
      was re-verified and found to be a necessary include with an
      accurate comment after architectural changes.

These changes address the specific points raised in the code review,
improving code structure, documentation, and cleanliness.
jonsimantov
jonsimantov commented Jun 17, 2025
View reviewed changes
@google-labs-jules
Fix: Remove SWIG_STORAGE_EXPORT from ListResult class
2041171 
This commit removes the SWIG_STORAGE_EXPORT macro from the
ListResult class declaration in
storage/src/include/firebase/storage/list_result.h,
as per code review feedback.

This change likely relates to managing symbol visibility or
discontinuing SWIG processing for this specific class.
@google-labs-jules
Fix: Remove SWIG macro from StorageReference List/ListAll & update Do…
12e873c 
…xygen

This commit addresses code review feedback:

1.  **Removed SWIG_STORAGE_EXPORT**: The `SWIG_STORAGE_EXPORT` macro
    was incorrectly added to the new `List()` and `ListAll()` method
    declarations in `firebase/storage/storage_reference.h`. This commit
    removes the macro from these methods.

2.  **Updated Doxygen for StorageReference List/ListAll**:
    The Doxygen comments for `StorageReference::List()`,
    `StorageReference::ListAll()` were updated to be more comprehensive.

3.  **Verified ListResult Doxygen Indentation**:
    The Doxygen comment indentation for methods in
    `firebase/storage/list_result.h` was reviewed and confirmed to be
    consistent with project standards. No changes were needed for indentation.
@jonsimantov jonsimantov changed the base branch from main to feature/storagereference_list June 17, 2025 23:21
@google-labs-jules
Fix: Correct include path for list_result.h in StorageReferenceInternal
b0050f0 
This commit corrects the #include path for the public
`firebase/storage/list_result.h` header within the desktop platform's
`StorageReferenceInternal` implementation
(`storage/src/desktop/storage_reference_desktop.cc`). I changed the path from a relative "firebase/storage/list_result.h" to a more
explicit "storage/src/include/firebase/storage/list_result.h" to
resolve a potential build issue.

Include paths for Android and iOS platforms were reviewed and found
to be correctly handling the inclusion of `firebase/storage/list_result.h`
via their respective `StorageReferenceInternal` header files using
standard include paths.

Additionally, I removed a minor extraneous agent-added comment
from `storage/src/ios/storage_reference_ios.h`.

I ran the code formatting script after these changes.
@jonsimantov jonsimantov added the skip-release-notes Skip release notes check label Jun 17, 2025
@google-labs-jules
Style: Perform deep cleanup of agent-added comments
e3bdc5f 
This commit applies a deep and thorough cleanup of comments
that I added during the implementation of the
ListResult and List/ListAll stub features. The focus was on
removing comments related to my iterative development
process, overly verbose explanations, and redundant notes,
particularly for #include directives.

This pass ensures that comments are concise, professional, and
add value rather than cluttering the code with development artifacts.
Pre-existing comments in the codebase were preserved.

Files affected by this deep cleanup include:
- storage/src/common/list_result.cc
- storage/src/include/firebase/storage/list_result.h
- storage/src/desktop/list_result_desktop.h
- storage/src/android/list_result_android.h
- storage/src/ios/list_result_ios.h
- storage/src/desktop/list_result_desktop.cc
- storage/src/android/list_result_android.cc
- storage/src/ios/list_result_ios.mm
- storage/src/common/storage_reference.cc
- storage/src/desktop/storage_reference_desktop.h
- storage/src/android/storage_reference_android.h
- storage/src/ios/storage_reference_ios.h
- storage/src/desktop/storage_reference_desktop.cc
- storage/src/android/storage_reference_android.cc
- storage/src/ios/storage_reference_ios.mm

The code formatting script `scripts/format_code.py -git_diff` was
run after these cleanups.
jonsimantov
jonsimantov commented Jun 18, 2025
View reviewed changes
storage/src/ios/storage_reference_ios.h

/// @brief Lists all items and prefixes under this reference (iOS implementation).
/// @return A Future that will be resolved with a ListResult.
virtual Future<ListResult> ListAll();
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No need for virtual functions anywhere here, there is no inheritance.

@jonsimantov jonsimantov changed the base branch from feature/storagereference_list to main June 25, 2025 20:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@github-actions github-actions[bot] github-actions[bot] left review comments

At least 1 approving review is required to merge this pull request.

Assignees

No one assigned

Labels

skip-release-notes Skip release notes check

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

1 participant

@jonsimantov