Repository navigation
GCP / Google workspace - #111
Conversation
…vates when a ConvertingFieldAccessor is encountered, existing accessor types are unaffected
# Conflicts: # connector/base/src/main/java/com/blazebit/query/connector/base/ConvertingFieldAccessor.java
There was a problem hiding this comment.
Code Review
The core wrapper pattern design is solid and consistent. Good test coverage for GCP Compute, Storage, and Google Workspace. The resolveType() / createConvertingAccessor() extension to ConventionContext is well-designed.
See inline comments for specific items to address.
| try { | ||
| List<Directory> directoryServices = GoogleDirectoryConnectorConfig.GOOGLE_DIRECTORY_SERVICE.getAll( context ); | ||
| List<Group> list = new ArrayList<>(); | ||
| List<GoogleGroup> list = new ArrayList<>(); |
There was a problem hiding this comment.
Potential NullPointerException: getGroups() can return null when no groups exist. The same issue applies to all other Google Workspace fetchers (MemberDataFetcher.getMembers(), UserDataFetcher.getUsers(), RoleDataFetcher.getItems(), RoleAssignmentDataFetcher.getItems(), DriveDataFetcher.getDrives()). Consider adding a null check or using a helper like:
List<Group> groups = directory.groups().list().setCustomer("my_customer").execute().getGroups();
if (groups !=
10BC8
null) {
for (Group group : groups) {
list.add(new GoogleGroup(group.getId(), group));
}
}| List<? extends GcpOrganization> organizations = context.getSession().getOrFetch( GcpOrganization.class ); | ||
| List<? extends GcpFolder> folders = context.getSession().getOrFetch( GcpFolder.class ); | ||
|
|
||
| for ( CredentialsProvider credentialsProvider : credentialsProviders ) { |
There was a problem hiding this comment.
Duplicate projects: This first calls client.searchProjects("") (returns all projects the caller can see), then also iterates organizations and folders via client.listProjects(parent). This will likely return duplicate projects. Either remove the search call, remove the org/folder iteration, or add deduplication logic.
| */ | ||
| package com.blazebit.query.connector.google.drive; | ||
|
|
||
| public abstract class GoogleWrapper<T> { |
There was a problem hiding this comment.
Duplicate class: This GoogleWrapper<T> is identical to com.blazebit.query.connector.google.directory.GoogleWrapper<T>. Consider extracting a single shared base class (e.g., in a google-base module or the existing connector-base) to avoid duplication.
| // queryContextBuilder.setProperty( GoogleDirectoryConnectorConfig.GOOGLE_DIRECTORY_SERVICE.getPropertyName(), createGoogleDirectory() ); | ||
| // queryContextBuilder.setProperty( GoogleDriveConnectorConfig.GOOGLE_DRIVE_SERVICE.getPropertyName(), createGoogleDrive() ); | ||
| // queryContextBuilder.setProperty( GcpConnectorConfig.GCP_CREDENTIALS_PROVIDER.getPropertyName(), createGcpCredentialsProvider() ); | ||
| queryContextBuilder.setProperty( GoogleDirectoryConnectorConfig.GOOGLE_DIRECTORY_SERVICE.getPropertyName(), createGoogleDirectory() ); |
There was a problem hiding this comment.
Debug leftover: These Google/GCP config lines appear to have been uncommented for local testing. They should be re-commented before merge, consistent with the other commented-out providers.
| import com.google.cloud.compute.v1.Instance; | ||
|
|
||
| public class GcpInstance extends GcpWrapper<Instance> { | ||
| public GcpInstance(String resourceId, Instance instance) { |
There was a problem hiding this comment.
Nit: extra space before GcpInstance:
public GcpInstance(String resourceId, Instance instance) {should be:
public GcpInstance(String resourceId, Instance instance) {| Expression e3 = toList( (CollectionDataFormat) dataFormatField.getFormat(), elementPhysType, e2, | ||
| subBlockBuilder ); | ||
| subBlockBuilder.add( Expressions.statement( Expressions.assign( resultVar, e3 ) ) ); | ||
| subBlockBuilder.add( Expressions.statement( Expressions.assign( resultVar, |
There was a problem hiding this comment.
This wrapping in FakeComparableList is a behavioral change in the core query engine. Could you add a brief comment explaining why this is needed? (e.g., is it for Calcite comparison operations on list types?)
| @Override | ||
| public Object get(Object o) { | ||
| try { | ||
| return converterMethod.invoke( null, delegate.get( o ) ); |
There was a problem hiding this comment.
Minor: the catch block is broad (Exception). Consider catching InvocationTargetException | IllegalAccessException specifically so unexpected runtime exceptions aren't silently wrapped.
| @@ -0,0 +1,158 @@ | |||
| /* | |||
There was a problem hiding this comment.
This test class (and GcpBucketTests, GoogleWorkSpaceTests, TestObjects) is in the default package (no package declaration). This works but is unconventional and can cause issues with some tooling. Consider adding a package.
- Make GoogleChromeOsDevice and GoogleMobileDevice extend GoogleWrapper for consistent resourceId/payload API across all connectors - Add workspace-base dependency to endpoint-verification module - Move test classes from default package to proper package - Update test queries to use payload accessor (d.payload.status) - Use device ID as resourceId instead of customer ID - Revert unrelated Hibernate version bump (will be separate PR)
No description provided.