Skip to content

File listing v2 - #21720

Merged
jaschrep-msft merged 16 commits into
Azure:feature/storage/stg78from
jaschrep-msft:file-listing-v2
Jun 17, 2021
Merged

File listing v2#21720
jaschrep-msft merged 16 commits into
Azure:feature/storage/stg78from
jaschrep-msft:file-listing-v2

Conversation

@jaschrep-msft

Copy link
Copy Markdown
Member

Incorporates new file and directory listing parameters and response fields for file shares.

regenerated files swagger for listing
hooked up generated and handwritten models
expanded client API to support new models
expanded test parameters for file listing.
swagger transform added.
@ghost ghost added the Storage Storage Service (Queues, Blobs, Files) label May 21, 2021
also fixed a method name casing
REST api accepts true or an absence of value. Alligning with .NET for
now on Boolean vs boolean.
@jaschrep-msft

Copy link
Copy Markdown
Member Author

Currently aligning with .NET on includeExtendedInfo being a Boolean vs a boolean. This should be discussed at the 78 API review.

@jaschrep-msft
jaschrep-msft marked this pull request as ready for review May 25, 2021 20:35
### Code generation settings
``` yaml
input-file: https://raw.githubusercontent.com/Azure/azure-rest-api-specs/storage-dataplane-preview/specification/storage/data-plane/Microsoft.FileStorage/preview/2020-04-08/file.json
input-file: https://raw.githubusercontent.com/seanmcc-msft/azure-rest-api-specs/feature/storage/listFilesV2/specification/storage/data-plane/Microsoft.FileStorage/preview/2020-10-02/file.json

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

revert this before merge (after making sure the swagger is merged too)

@gapra-msft gapra-msft left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

lgtm. I have a few design related comments

@jaschrep-msft
jaschrep-msft merged commit a13f37f into Azure:feature/storage/stg78 Jun 17, 2021
final ShareListFilesAndDirectoriesOptions modifiedOptions = options == null
? new ShareListFilesAndDirectoriesOptions() : options;

List<ListFilesIncludeType> includeTypes = new LinkedList<>();

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I'm not sure why we'd choose LinkedList here. It should matter much with this size of data but we should by default pick ArrayList unless we hava a good reason to use LinkedList.

* @param lastModified Datetime the item was last modified.
* @param eTag ETag of the item.
*/
public ShareFileItemProperties(OffsetDateTime createdOn, OffsetDateTime lastAccessedOn,

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

If this is new type then I'd recommend to convert it into interface and have actual implementation in the "implementation" package. This will help us manage growing model without worry about breaking ctors.

Let's do this for all immutable models we add.

Comment on lines +85 to +88
if (includeTimestamps || includeETag || includeAttributes || includePermissionKey) {
throw logger.logExceptionAsError(
new IllegalStateException("includeExtendedInfo must be true in the current state."));
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

we should remove this validation. service makes sure data is pulled from right place implicitly if extra attributes are requested. there's no need for us validating this.

Comment on lines +106 to +108
if (includeTimestamps) {
this.includeExtendedInfo = true;
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This isn't necessary as well. service will do the right thing. Please remove this and similar logic.

Comment on lines +403 to +411
def options = new ShareListFilesAndDirectoriesOptions()
.setPrefix(namer.getResourcePrefix() + extraPrefix)
.setMaxResultsPerPage(maxResults)
.setIncludeExtendedInfo(includeExtendedInfo) // set FIRST since subsequent can autoset this one
.setIncludeTimestamps(timestamps)
.setIncludeETag(etag)
.setIncludeAttributes(attributes)
.setIncludePermissionKey(permissionKey)
def returnedFileList = primaryDirectoryClient.listFilesAndDirectories(options, null, null).collect()

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

let's keep tests with extended and without extended info separate. We'd like to run existing form of test for older service versions - if we mix them together we'll loose that ability.

You may need to annotate tests that take extended info with @RequiredServiceVersion annotation.

kasobol-msft added a commit that referenced this pull request Jun 24, 2021
* Set up STG78 branch (#21634)

* Generated off latest version of blob swagger (#21644)

* Reenabled quick query parquet support (#21785)

* Implemented Immutable Storage with Versioning (#21718)

* File listing v2 (#21720)

* swagger generation and hookup

regenerated files swagger for listing
hooked up generated and handwritten models
expanded client API to support new models

* testing and fixes

expanded test parameters for file listing.
swagger transform added.

* rerecorded file listing test

* more testing

also fixed a method name casing

* docstrings and samples

* checkstyle

* reverted primative boolean extended info arg

REST api accepts true or an absence of value. Alligning with .NET for
now on Boolean vs boolean.

* checkpoint

* pr feedback

* misused variable fix

* fixed test

* send nothing, not false: include-extended-info

other minor fixes
rerecorded tests

* import cleanup

* swagger regeneration and test fixes

* API comments

extendedInfo boolean => Boolean
renamed options class

* reidentified samples for options class name change

Co-authored-by: jschrepp-MSFT <41338290+jschrepp-MSFT@users.noreply.github.com>

* Fix test.

* Implemented list deleted root blob with versions (#21996)

* Addressed comments for immutable storage with versioning (#22388)

* Develop on main, part 1 (#22451)

* move back service version.

* pin CI.

* fix ci.

* fix ci ?

* fix live tests ?

* use latest service version to filter if not specified in variable.

* use latest service version to filter if not specified in variable.

* does this work ??

* missing stuff.

* move that piece to parent as well.

* rollback yml changes in favor of pom.xml

* fix build ?

* how about this ?

* more ?

* not this.

* rollback some.

* does this work ??

Co-authored-by: Gauri Prasad <51212198+gapra-msft@users.noreply.github.com>
Co-authored-by: gapra <gapra@microsoft.com>
Co-authored-by: James <41338290+jaschrep-msft@users.noreply.github.com>
Co-authored-by: jschrepp-MSFT <41338290+jschrepp-MSFT@users.noreply.github.com>
jaschrep-msft added a commit that referenced this pull request Jun 25, 2021
* Set up STG78 branch (#21634)

* Generated off latest version of blob swagger (#21644)

* swagger generation and public API overloads

* blob clients obey new generation

* minor changes to bearer token logic

* TEST WORKING

* extra testing

* ported oauthcopy source to blobs

* options API changes and docstrings

* fixed constructor issues

* fix missed source auth misses

* script tags on pom

* removes arg checks in option model constructors

rerecorded tests

* pom fix

* fix misnamed page blob tests

* Test fixes

adds missing recording.
extra playback logic for oauth tokens that should fix a CI-specific
issue.

* fixed pom script comment

* downgrade some depenency verions

* rerecord sas tests

* snippets and checkstyle

* rerecorded tests

fixed recording infra for file share tests

* test fixes

* rerecorded versioning tests

* HttpAuthorization added to core-experimental

azure-storage-blob and azure-storage-file-share now depend on
core-experimental for new class and updated their APIs to use the new
class.

* refactored options type names

* Reenabled quick query parquet support (#21785)

* reidentified samples for options class name change

* Implemented Immutable Storage with Versioning (#21718)

* File listing v2 (#21720)

* swagger generation and hookup

regenerated files swagger for listing
hooked up generated and handwritten models
expanded client API to support new models

* testing and fixes

expanded test parameters for file listing.
swagger transform added.

* rerecorded file listing test

* more testing

also fixed a method name casing

* docstrings and samples

* checkstyle

* reverted primative boolean extended info arg

REST api accepts true or an absence of value. Alligning with .NET for
now on Boolean vs boolean.

* checkpoint

* pr feedback

* misused variable fix

* fixed test

* send nothing, not false: include-extended-info

other minor fixes
rerecorded tests

* import cleanup

* swagger regeneration and test fixes

* API comments

extendedInfo boolean => Boolean
renamed options class

* reidentified samples for options class name change

Co-authored-by: jschrepp-MSFT <41338290+jschrepp-MSFT@users.noreply.github.com>

* Fix test.

* remove HttpAuthorization in prep for merge

added in separate PR. removing to be ready for that offical addition.

* fix build from merge

* fix pom versioning CI issues

* snippet line length

* fix merge error with immutable versioning

* PR feedback

* Minor cleanup

* generator cleanup and regeneration

* Keep files-shares tests dependent on latest blob

* test refactor/rerecord

* deleted old tests that were copied instead of moved

* min service version reformat

* import cleanup

* post-merge.

* getOauthToken refactor

* reformatting

Co-authored-by: Gauri Prasad <51212198+gapra-msft@users.noreply.github.com>
Co-authored-by: jschrepp-MSFT <41338290+jschrepp-MSFT@users.noreply.github.com>
Co-authored-by: gapra <gapra@microsoft.com>
Co-authored-by: Kamil Sobol <kasobol@microsoft.com>
Co-authored-by: Kamil Sobol <61715331+kasobol-msft@users.noreply.github.com>
@jaschrep-msft
jaschrep-msft deleted the file-listing-v2 branch August 3, 2022 15:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Storage Storage Service (Queues, Blobs, Files)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants