|
|
DescriptionFetch files shared in Team Drives by specifying allTeamDrives corpora.
In order to list all files shared by Team Drive, "corpora" parameter
should be set in addition to "includeTeamDriveItems".
https://developers.google.com/drive/v2/reference/files/list
For efficiency we should use teamDrive corpora when it only need files in a single Team Drive. (e.g. when getting list of files under a single directory.) It will need to change existing code that hadn't considered such distinction. So this change will use allTeamDrives corpora as a tentative fix, when Team Drives integration is enabled.
It will not cause performance regression when Team Drives integration is disabled.
TEST=unit_tests --gtest_filter=DriveApiUrlGeneratorTest.*
BUG=723955
Review-Url: https://codereview.chromium.org/2894513003
Cr-Commit-Position: refs/heads/master@{#476951}
Committed: https://chromium.googlesource.com/chromium/src/+/e6d18b1ce2297312eb28235f0bbf501fea53d627
Patch Set 1 #
Total comments: 2
Patch Set 2 : Allow setting team_drive_id and corpora parameters #Patch Set 3 : Rename the new class to FileListScope. Eliminate unrelated changes. #Patch Set 4 : Rename parameter variables to scope #
Total comments: 8
Patch Set 5 : Return reference to scope_ with the getter. #Patch Set 6 : Use pair of corpora enum and team drive ID instead of new class. #Patch Set 7 : Eliminate variables of class type with static storage duration. #
Total comments: 12
Patch Set 8 : Address review comments. #
Total comments: 4
Patch Set 9 : Use class enum and add comments. #Patch Set 10 : Make comment style consistent with existing one #Messages
Total messages: 55 (39 generated)
The CQ bit was checked by yamaguchi@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== Add corpora parameter to include Team Drive files in Files list. TEST=unit_tests --gtest_filter=DriveApiUrlGeneratorTest.* BUG=723955 ========== to ========== Fetch files shared in Team Drives by the right corpora. https://developers.google.com/drive/v2/reference/files/list TEST=unit_tests --gtest_filter=DriveApiUrlGeneratorTest.* BUG=723955 ==========
Description was changed from ========== Fetch files shared in Team Drives by the right corpora. https://developers.google.com/drive/v2/reference/files/list TEST=unit_tests --gtest_filter=DriveApiUrlGeneratorTest.* BUG=723955 ========== to ========== Fetch files shared in Team Drives by specifying the right corpora. In order to list files shared by Team Drive, "corpora" parameter should be set in addition to "includeTeamDriveItems". https://developers.google.com/drive/v2/reference/files/list TEST=unit_tests --gtest_filter=DriveApiUrlGeneratorTest.* BUG=723955 ==========
yamaguchi@chromium.org changed reviewers: + hashimoto@chromium.org
Will you take a look at this first? This is a fix for the full fetch of the files. On the other hand, the other ongoing review, crrev/2885323002, is for diff update of the files. Each component can be fixed independently.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2894513003/diff/1/google_apis/drive/drive_api... File google_apis/drive/drive_api_url_generator.cc (right): https://codereview.chromium.org/2894513003/diff/1/google_apis/drive/drive_api... google_apis/drive/drive_api_url_generator.cc:196: "default,allTeamDrives"); 1. Is allTeamDrives the best option we can use here? The reference says you should avoid using allTeamDrives. 2. What is the difference between this "corpora" parameter and "fields" parameter?
> 1. Is allTeamDrives the best option we can use here? The reference says you > should avoid using allTeamDrives. > I agree that use of allTeamDrives should be limited only when truly use all data returned (when doing full fetch). Currently we are also using this function just to fetch a single directory, when it won't require allTeamDrives corpus. Therefore I will revise this change to take the parameter to allow choosing the corpus. > > 2. What is the difference between this "corpora" parameter and "fields" > parameter? "corpora" limits the subset of entries to be looked up, which will be split by each team drive. {default, allTeamDrives, teamDrive}. "fields" limits the subset of fields which should be included in the response. e.g. "nextPageToken,items(title,id)".
The CQ bit was checked by yamaguchi@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by yamaguchi@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== Fetch files shared in Team Drives by specifying the right corpora. In order to list files shared by Team Drive, "corpora" parameter should be set in addition to "includeTeamDriveItems". https://developers.google.com/drive/v2/reference/files/list TEST=unit_tests --gtest_filter=DriveApiUrlGeneratorTest.* BUG=723955 ========== to ========== Fetch files shared in Team Drives by specifying allTeamDrives corpora. In order to list files shared by Team Drive, "corpora" parameter should be set in addition to "includeTeamDriveItems". https://developers.google.com/drive/v2/reference/files/list For efficiency we should use teamDrive corpora when it only need files in a single Team Drive. (e.g. when getting list of files under a single directory.) It will need to change existing code that hadn't considered such distinction. So this change will use allTeamDrives corpora as a tentative fix, when Team Drives integration is enabled. It will not cause performance regression when Team Drives integration is disabled. TEST=unit_tests --gtest_filter=DriveApiUrlGeneratorTest.* BUG=723955 ==========
Description was changed from ========== Fetch files shared in Team Drives by specifying allTeamDrives corpora. In order to list files shared by Team Drive, "corpora" parameter should be set in addition to "includeTeamDriveItems". https://developers.google.com/drive/v2/reference/files/list For efficiency we should use teamDrive corpora when it only need files in a single Team Drive. (e.g. when getting list of files under a single directory.) It will need to change existing code that hadn't considered such distinction. So this change will use allTeamDrives corpora as a tentative fix, when Team Drives integration is enabled. It will not cause performance regression when Team Drives integration is disabled. TEST=unit_tests --gtest_filter=DriveApiUrlGeneratorTest.* BUG=723955 ========== to ========== Fetch files shared in Team Drives by specifying allTeamDrives corpora. In order to list all files shared by Team Drive, "corpora" parameter should be set in addition to "includeTeamDriveItems". https://developers.google.com/drive/v2/reference/files/list For efficiency we should use teamDrive corpora when it only need files in a single Team Drive. (e.g. when getting list of files under a single directory.) It will need to change existing code that hadn't considered such distinction. So this change will use allTeamDrives corpora as a tentative fix, when Team Drives integration is enabled. It will not cause performance regression when Team Drives integration is disabled. TEST=unit_tests --gtest_filter=DriveApiUrlGeneratorTest.* BUG=723955 ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by yamaguchi@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2894513003/diff/1/google_apis/drive/drive_api... File google_apis/drive/drive_api_url_generator.cc (right): https://codereview.chromium.org/2894513003/diff/1/google_apis/drive/drive_api... google_apis/drive/drive_api_url_generator.cc:196: "default,allTeamDrives"); On 2017/05/19 11:14:26, hashimoto wrote: > 1. Is allTeamDrives the best option we can use here? The reference says you > should avoid using allTeamDrives. Revised it to have a parameter for specifying the range of query, so that this can use teamDrive corpora when it's more appropriate. However the logic of callers should be changed to use it. I left TODOs for those places. > > 2. What is the difference between this "corpora" parameter and "fields" > parameter?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Sorry for belated response. I still don't understand how this code will be used. How will the interfaces of JobScheduler and DriveAPIService look like? https://codereview.chromium.org/2894513003/diff/60001/google_apis/drive/drive... File google_apis/drive/drive_api_requests.h (right): https://codereview.chromium.org/2894513003/diff/60001/google_apis/drive/drive... google_apis/drive/drive_api_requests.h:535: const FilesListScope scope() const { return scope_; } This should be a const reference. const FilesListScope& https://codereview.chromium.org/2894513003/diff/60001/google_apis/drive/drive... File google_apis/drive/drive_api_url_generator.cc (right): https://codereview.chromium.org/2894513003/diff/60001/google_apis/drive/drive... google_apis/drive/drive_api_url_generator.cc:403: std::string FilesListScope::kDefaultAndAllTeamDrivesCorpus_ = Variables of class type with static storage duration are forbidden. https://google.github.io/styleguide/cppguide.html#Static_and_Global_Variables https://codereview.chromium.org/2894513003/diff/60001/google_apis/drive/drive... File google_apis/drive/drive_api_url_generator.h (right): https://codereview.chromium.org/2894513003/diff/60001/google_apis/drive/drive... google_apis/drive/drive_api_url_generator.h:19: class FilesListScope { Sorry, I couldn't understand why this needs to be a class. Can't this be just a std::string for Team Drive ID, and an enum for the corpora?
On 2017/05/26 09:09:19, hashimoto wrote: > Sorry for belated response. > I still don't understand how this code will be used. > How will the interfaces of JobScheduler and DriveAPIService look like? > My current idea is: - {JobScheduler,DriveAPIService}::GetFileListInDirectory will require a string |team_drive_id| as an additional param. - DirectoryLoader will be responsible to generate team_drive_id and pass to JobScheduler::GetFileListInDirectory. - {JobScheduler,DriveAPIService}::GetAllFileList will not change their signatures. (always fetch all team drives) - DriveAPIService::GetAllFileList will pass kAllTeamDrives to DriveAPIService::GetFileListInDirectory. Here is a next change being developed. https://codereview.chromium.org/2910913002/
https://codereview.chromium.org/2894513003/diff/60001/google_apis/drive/drive... File google_apis/drive/drive_api_requests.h (right): https://codereview.chromium.org/2894513003/diff/60001/google_apis/drive/drive... google_apis/drive/drive_api_requests.h:535: const FilesListScope scope() const { return scope_; } On 2017/05/26 09:09:19, hashimoto wrote: > This should be a const reference. > const FilesListScope& Done. https://codereview.chromium.org/2894513003/diff/60001/google_apis/drive/drive... File google_apis/drive/drive_api_url_generator.h (right): https://codereview.chromium.org/2894513003/diff/60001/google_apis/drive/drive... google_apis/drive/drive_api_url_generator.h:19: class FilesListScope { On 2017/05/26 09:09:19, hashimoto wrote: > Sorry, I couldn't understand why this needs to be a class. > Can't this be just a std::string for Team Drive ID, and an enum for the corpora? I think it can. - corpora : enum - team_drive_id : std::string. Ignored if corpus is not teamDrive. The only reason to have this class is to make sure callers won't need to pass an unused dummy string as teamDrive in case of allTeamDrives or default corpora. If you think this is not much benefit, I will replace this with such 2 parameters (with the notice above in @param document).
BTW, could you answer to the questions on the design doc you shared offline? https://codereview.chromium.org/2894513003/diff/60001/google_apis/drive/drive... File google_apis/drive/drive_api_url_generator.h (right): https://codereview.chromium.org/2894513003/diff/60001/google_apis/drive/drive... google_apis/drive/drive_api_url_generator.h:19: class FilesListScope { On 2017/05/29 08:38:21, yamaguchi wrote: > On 2017/05/26 09:09:19, hashimoto wrote: > > Sorry, I couldn't understand why this needs to be a class. > > Can't this be just a std::string for Team Drive ID, and an enum for the > corpora? > > I think it can. > - corpora : enum > - team_drive_id : std::string. Ignored if corpus is not teamDrive. > The only reason to have this class is to make sure callers won't need to pass an > unused dummy string as teamDrive in case of allTeamDrives or default corpora. If > you think this is not much benefit, I will replace this with such 2 parameters > (with the notice above in @param document). IMO the Drive API is already too complicated (it's not easy to describe the relationships between corpora, corpus, includeTeamDriveItems, supportsTeamDrives, and teamDriveId). Introducing a new "scope" concept (btw "scope" is a term already used by OAuth) just makes the situation worse. I think it's better to keep our code as dumb as possible.
The CQ bit was checked by yamaguchi@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by yamaguchi@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2894513003/diff/60001/google_apis/drive/drive... File google_apis/drive/drive_api_url_generator.cc (right): https://codereview.chromium.org/2894513003/diff/60001/google_apis/drive/drive... google_apis/drive/drive_api_url_generator.cc:403: std::string FilesListScope::kDefaultAndAllTeamDrivesCorpus_ = On 2017/05/26 09:09:19, hashimoto wrote: > Variables of class type with static storage duration are forbidden. > https://google.github.io/styleguide/cppguide.html#Static_and_Global_Variables Done. https://codereview.chromium.org/2894513003/diff/60001/google_apis/drive/drive... File google_apis/drive/drive_api_url_generator.h (right): https://codereview.chromium.org/2894513003/diff/60001/google_apis/drive/drive... google_apis/drive/drive_api_url_generator.h:19: class FilesListScope { On 2017/05/30 08:34:57, hashimoto wrote: > On 2017/05/29 08:38:21, yamaguchi wrote: > > On 2017/05/26 09:09:19, hashimoto wrote: > > > Sorry, I couldn't understand why this needs to be a class. > > > Can't this be just a std::string for Team Drive ID, and an enum for the > > corpora? > > > > I think it can. > > - corpora : enum > > - team_drive_id : std::string. Ignored if corpus is not teamDrive. > > The only reason to have this class is to make sure callers won't need to pass > an > > unused dummy string as teamDrive in case of allTeamDrives or default corpora. > If > > you think this is not much benefit, I will replace this with such 2 parameters > > (with the notice above in @param document). > > IMO the Drive API is already too complicated (it's not easy to describe the > relationships between corpora, corpus, includeTeamDriveItems, > supportsTeamDrives, and teamDriveId). > Introducing a new "scope" concept (btw "scope" is a term already used by OAuth) > just makes the situation worse. > > I think it's better to keep our code as dumb as possible. Thanks. Changed to use 2 args instead of a new class.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Sorry for being late to respond. https://codereview.chromium.org/2894513003/diff/120001/components/drive/servi... File components/drive/service/drive_api_service.cc (right): https://codereview.chromium.org/2894513003/diff/120001/components/drive/servi... components/drive/service/drive_api_service.cc:381: kMaxNumFilesResourcePerRequest, corpora, "", Use std::string() instead "" to avoid char*->std::string conversion. The same goes for others. https://codereview.chromium.org/2894513003/diff/120001/google_apis/drive/driv... File google_apis/drive/drive_api_requests.h (right): https://codereview.chromium.org/2894513003/diff/120001/google_apis/drive/driv... google_apis/drive/drive_api_requests.h:536: void set_corpora(const FilesListCorpora& corpora) { corpora_ = corpora; } const FilesListCorpora& -> FilesListCorpora. enum is a scalar type. https://codereview.chromium.org/2894513003/diff/120001/google_apis/drive/driv... File google_apis/drive/drive_api_url_generator.cc (right): https://codereview.chromium.org/2894513003/diff/120001/google_apis/drive/driv... google_apis/drive/drive_api_url_generator.cc:200: net::AppendOrReplaceQueryParameter(url, kIncludeTeamDriveItems, "true"); Are kSupportsTeamDrives and kIncludeTeamDriveItems needed only when the corpora doesn't include Team Drives? https://codereview.chromium.org/2894513003/diff/120001/google_apis/drive/driv... google_apis/drive/drive_api_url_generator.cc:413: default: Please have cases for all possible enums. Instead of default:, you can add NOTREACHED to after this switch. https://codereview.chromium.org/2894513003/diff/120001/google_apis/drive/driv... File google_apis/drive/drive_api_url_generator.h (right): https://codereview.chromium.org/2894513003/diff/120001/google_apis/drive/driv... google_apis/drive/drive_api_url_generator.h:77: FilesListCorpora corpora, How about moving FilesListCorpora enum to drive_api_requests.h (probably inside FilesListRequest), and making this function take std::string? https://codereview.chromium.org/2894513003/diff/120001/google_apis/drive/driv... google_apis/drive/drive_api_url_generator.h:142: static const char* GetCorporaString_(FilesListCorpora corpora); This should be a free function in an unnamed namespace.
The CQ bit was checked by yamaguchi@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2894513003/diff/120001/components/drive/servi... File components/drive/service/drive_api_service.cc (right): https://codereview.chromium.org/2894513003/diff/120001/components/drive/servi... components/drive/service/drive_api_service.cc:381: kMaxNumFilesResourcePerRequest, corpora, "", On 2017/06/02 09:44:46, hashimoto wrote: > Use std::string() instead "" to avoid char*->std::string conversion. > The same goes for others. Done. https://codereview.chromium.org/2894513003/diff/120001/google_apis/drive/driv... File google_apis/drive/drive_api_requests.h (right): https://codereview.chromium.org/2894513003/diff/120001/google_apis/drive/driv... google_apis/drive/drive_api_requests.h:536: void set_corpora(const FilesListCorpora& corpora) { corpora_ = corpora; } On 2017/06/02 09:44:46, hashimoto wrote: > const FilesListCorpora& -> FilesListCorpora. > > enum is a scalar type. Done. https://codereview.chromium.org/2894513003/diff/120001/google_apis/drive/driv... File google_apis/drive/drive_api_url_generator.cc (right): https://codereview.chromium.org/2894513003/diff/120001/google_apis/drive/driv... google_apis/drive/drive_api_url_generator.cc:200: net::AppendOrReplaceQueryParameter(url, kIncludeTeamDriveItems, "true"); On 2017/06/02 09:44:46, hashimoto wrote: > Are kSupportsTeamDrives and kIncludeTeamDriveItems needed only when the corpora > doesn't include Team Drives? Changed to turn off includeTeamDriveItems for the default corpus. supportsTeamDrives is perhaps not needed especially for this case, but I think we probably should have this parameter because this is a parameter to indicate the client application is aware of (=designed to handle response related to) Team Drives. The reference in the API document says: > Whether the requesting application supports Team Drives https://codereview.chromium.org/2894513003/diff/120001/google_apis/drive/driv... google_apis/drive/drive_api_url_generator.cc:413: default: On 2017/06/02 09:44:46, hashimoto wrote: > Please have cases for all possible enums. > Instead of default:, you can add NOTREACHED to after this switch. Done. https://codereview.chromium.org/2894513003/diff/120001/google_apis/drive/driv... File google_apis/drive/drive_api_url_generator.h (right): https://codereview.chromium.org/2894513003/diff/120001/google_apis/drive/driv... google_apis/drive/drive_api_url_generator.h:77: FilesListCorpora corpora, On 2017/06/02 09:44:46, hashimoto wrote: > How about moving FilesListCorpora enum to drive_api_requests.h (probably inside > FilesListRequest), and making this function take std::string? I think it'd be better to have this parameter an enum because we'll have a conditional statement that switches behavior depending on the meaning of an enum value. https://codereview.chromium.org/2894513003/diff/120001/google_apis/drive/driv... google_apis/drive/drive_api_url_generator.h:142: static const char* GetCorporaString_(FilesListCorpora corpora); On 2017/06/02 09:44:46, hashimoto wrote: > This should be a free function in an unnamed namespace. Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Almost looking good https://codereview.chromium.org/2894513003/diff/140001/google_apis/drive/driv... File google_apis/drive/drive_api_url_generator.h (right): https://codereview.chromium.org/2894513003/diff/140001/google_apis/drive/driv... google_apis/drive/drive_api_url_generator.h:18: enum FilesListCorpora { Please add comments to describe what this enum is for, and what each enum value means. https://codereview.chromium.org/2894513003/diff/140001/google_apis/drive/driv... google_apis/drive/drive_api_url_generator.h:19: CORPORA_DEFAULT, How about making this an enum class? enum class FilesListCorpora { DEFAULT, TEAM_DRIVE, ALL_TEAM_DRIVES, };
The CQ bit was checked by yamaguchi@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2894513003/diff/140001/google_apis/drive/driv... File google_apis/drive/drive_api_url_generator.h (right): https://codereview.chromium.org/2894513003/diff/140001/google_apis/drive/driv... google_apis/drive/drive_api_url_generator.h:18: enum FilesListCorpora { On 2017/06/05 04:29:22, hashimoto wrote: > Please add comments to describe what this enum is for, and what each enum value > means. Done. https://codereview.chromium.org/2894513003/diff/140001/google_apis/drive/driv... google_apis/drive/drive_api_url_generator.h:19: CORPORA_DEFAULT, On 2017/06/05 04:29:22, hashimoto wrote: > How about making this an enum class? > > enum class FilesListCorpora { > DEFAULT, > TEAM_DRIVE, > ALL_TEAM_DRIVES, > }; Done.
lgtm
The CQ bit was unchecked by yamaguchi@chromium.org
The CQ bit was checked by yamaguchi@chromium.org
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch. Bot data: {"patchset_id": 180001, "attempt_start_ts": 1496643397138920, "parent_rev": "ca0bc1dcb2a5a4d3125e06d0e255b39a0eae431e", "commit_rev": "e6d18b1ce2297312eb28235f0bbf501fea53d627"}
Message was sent while issue was closed.
Description was changed from ========== Fetch files shared in Team Drives by specifying allTeamDrives corpora. In order to list all files shared by Team Drive, "corpora" parameter should be set in addition to "includeTeamDriveItems". https://developers.google.com/drive/v2/reference/files/list For efficiency we should use teamDrive corpora when it only need files in a single Team Drive. (e.g. when getting list of files under a single directory.) It will need to change existing code that hadn't considered such distinction. So this change will use allTeamDrives corpora as a tentative fix, when Team Drives integration is enabled. It will not cause performance regression when Team Drives integration is disabled. TEST=unit_tests --gtest_filter=DriveApiUrlGeneratorTest.* BUG=723955 ========== to ========== Fetch files shared in Team Drives by specifying allTeamDrives corpora. In order to list all files shared by Team Drive, "corpora" parameter should be set in addition to "includeTeamDriveItems". https://developers.google.com/drive/v2/reference/files/list For efficiency we should use teamDrive corpora when it only need files in a single Team Drive. (e.g. when getting list of files under a single directory.) It will need to change existing code that hadn't considered such distinction. So this change will use allTeamDrives corpora as a tentative fix, when Team Drives integration is enabled. It will not cause performance regression when Team Drives integration is disabled. TEST=unit_tests --gtest_filter=DriveApiUrlGeneratorTest.* BUG=723955 Review-Url: https://codereview.chromium.org/2894513003 Cr-Commit-Position: refs/heads/master@{#476951} Committed: https://chromium.googlesource.com/chromium/src/+/e6d18b1ce2297312eb28235f0bbf... ==========
Message was sent while issue was closed.
Committed patchset #10 (id:180001) as https://chromium.googlesource.com/chromium/src/+/e6d18b1ce2297312eb28235f0bbf... |