Skip to content
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.

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

Fix NPE in string function on DatabricksConfig #285

Merged
merged 2 commits into from
May 13, 2024
Merged

Conversation

pietern
Copy link
Contributor

@pietern pietern commented May 8, 2024

Changes

If the configuration isn't resolved, the env field will be null, and the toString function would throw an NPE. This change fixes that and adds a few tests. I changed the string output to always be non-empty, even if no properties are set.

Tests

Unit tests.

@pietern pietern requested review from mgyucht and tanmay-db May 8, 2024 13:55
@@ -197,7 +198,13 @@ public static String debugString(DatabricksConfig cfg) {
List<String> attrsUsed = new ArrayList<>();
List<String> buf = new ArrayList<>();

Map<String, String> getEnvAllEnv = cfg.getEnv().getEnv();
Environment env = cfg.getEnv();
Copy link
Contributor

Choose a reason for hiding this comment

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

If my understanding is correct, then this would only happen for when we do config.toString() since we directly call debugString, for the other case (where we call debugString in makeNicerError) we have a check that checks for the null env:

    if (cfg.getEnv() != null) {
      debugString = debugString(cfg);
    }

Since we are putting a null check at the base (in debugString), we might have to remove this check in makeNicerError and update the if condition on line 189, since debugString would never be empty: !debugString.isEmpty()

Otherwise the message that we get from makeNicerError won't contain Env: <none>, Config: <empty>

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think we need to change anything about that other callsite. Basically, it's saying that we won't include the config in the makeNicerError message if it is not yet resolved, but we can still include that in DatabricksConfig's toString()

@@ -197,7 +198,13 @@ public static String debugString(DatabricksConfig cfg) {
List<String> attrsUsed = new ArrayList<>();
List<String> buf = new ArrayList<>();

Map<String, String> getEnvAllEnv = cfg.getEnv().getEnv();
Environment env = cfg.getEnv();
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think we need to change anything about that other callsite. Basically, it's saying that we won't include the config in the makeNicerError message if it is not yet resolved, but we can still include that in DatabricksConfig's toString()

@mgyucht mgyucht added this pull request to the merge queue May 13, 2024
Merged via the queue into main with commit f1d1714 May 13, 2024
9 checks passed
@mgyucht mgyucht deleted the config-tostring branch May 13, 2024 11:21
tanmay-db added a commit that referenced this pull request May 15, 2024
* Fix OIDC Endpoint Fetching in DatabricksConfig for Workspace Clients ([#277](#277)).
* Fix NullPointerException when reading error response body ([#276](#276)).
* Update SDK to OpenAPI spec ([#280](#280)).
* Remove unused script from repository root ([#281](#281)).
* Add missing return ([#283](#283)).
* Incorporate host in request after `authenticate()` call ([#282](#282)).
* Fix NPE in string function on DatabricksConfig ([#285](#285)).
* Add instructions for building a shaded JAR ([#284](#284)).
* Fix test that was picking up configuration from the environment ([#287](#287)).

API Changes:

 * Added `ingestionDefinition` field for `com.databricks.sdk.service.pipelines.CreatePipeline`.
 * Added `ingestionDefinition` field for `com.databricks.sdk.service.pipelines.EditPipeline`.
 * Added `ingestionDefinition` field for `com.databricks.sdk.service.pipelines.PipelineSpec`.
 * Added `com.databricks.sdk.service.pipelines.IngestionConfig` class.
 * Added `com.databricks.sdk.service.pipelines.ManagedIngestionPipelineDefinition` class.
 * Added `com.databricks.sdk.service.pipelines.SchemaSpec` class.
 * Added `com.databricks.sdk.service.pipelines.TableSpec` class.
 * Changed `create()` method for `workspaceClient.apps()` service . New request type is `com.databricks.sdk.service.serving.CreateAppRequest` class.
 * Changed `create()` method for `workspaceClient.apps()` service to return `com.databricks.sdk.service.serving.App` class.
 * Removed `deleteApp()` method for `workspaceClient.apps()` service.
 * Removed `getApp()` method for `workspaceClient.apps()` service.
 * Removed `getAppDeploymentStatus()` method for `workspaceClient.apps()` service.
 * Removed `getApps()` method for `workspaceClient.apps()` service.
 * Removed `getEvents()` method for `workspaceClient.apps()` service.
 * Added `createDeployment()` method for `workspaceClient.apps()` service.
 * Added `delete()` method for `workspaceClient.apps()` service.
 * Added `get()` method for `workspaceClient.apps()` service.
 * Added `getDeployment()` method for `workspaceClient.apps()` service.
 * Added `getEnvironment()` method for `workspaceClient.apps()` service.
 * Added `list()` method for `workspaceClient.apps()` service.
 * Added `listDeployments()` method for `workspaceClient.apps()` service.
 * Added `stop()` method for `workspaceClient.apps()` service.
 * Added `update()` method for `workspaceClient.apps()` service.
 * Added `getOpenApi()` method for `workspaceClient.servingEndpoints()` service.
 * Removed `com.databricks.sdk.service.serving.AppEvents` class.
 * Removed `com.databricks.sdk.service.serving.AppManifest` class.
 * Removed `com.databricks.sdk.service.serving.AppServiceStatus` class.
 * Added `routeOptimized` field for `com.databricks.sdk.service.serving.CreateServingEndpoint`.
 * Removed `com.databricks.sdk.service.serving.DeleteAppResponse` class.
 * Removed `com.databricks.sdk.service.serving.DeployAppRequest` class.
 * Removed `com.databricks.sdk.service.serving.DeploymentStatus` class.
 * Removed `com.databricks.sdk.service.serving.DeploymentStatusState` class.
 * Removed `com.databricks.sdk.service.serving.GetAppDeploymentStatusRequest` class.
 * Removed `com.databricks.sdk.service.serving.GetAppResponse` class.
 * Removed `com.databricks.sdk.service.serving.GetEventsRequest` class.
 * Removed `com.databricks.sdk.service.serving.ListAppEventsResponse` class.
 * Changed `apps` field for `com.databricks.sdk.service.serving.ListAppsResponse` to `com.databricks.sdk.service.serving.AppList` class.
 * Added `endpointUrl` field for `com.databricks.sdk.service.serving.ServingEndpointDetailed`.
 * Added `routeOptimized` field for `com.databricks.sdk.service.serving.ServingEndpointDetailed`.
 * Added `com.databricks.sdk.service.serving.App` class.
 * Added `com.databricks.sdk.service.serving.AppDeployment` class.
 * Added `com.databricks.sdk.service.serving.AppDeploymentState` class.
 * Added `com.databricks.sdk.service.serving.AppDeploymentStatus` class.
 * Added `com.databricks.sdk.service.serving.AppEnvironment` class.
 * Added `com.databricks.sdk.service.serving.AppState` class.
 * Added `com.databricks.sdk.service.serving.AppStatus` class.
 * Added `com.databricks.sdk.service.serving.CreateAppDeploymentRequest` class.
 * Added `com.databricks.sdk.service.serving.CreateAppRequest` class.
 * Added `com.databricks.sdk.service.serving.EnvVariable` class.
 * Added `com.databricks.sdk.service.serving.GetAppDeploymentRequest` class.
 * Added `com.databricks.sdk.service.serving.GetAppEnvironmentRequest` class.
 * Added `com.databricks.sdk.service.serving.GetOpenApiRequest` class.
 * Added `Object` class.
 * Added `com.databricks.sdk.service.serving.ListAppDeploymentsRequest` class.
 * Added `com.databricks.sdk.service.serving.ListAppDeploymentsResponse` class.
 * Added `com.databricks.sdk.service.serving.ListAppsRequest` class.
 * Added `com.databricks.sdk.service.serving.StopAppRequest` class.
 * Added `Object` class.
 * Added `com.databricks.sdk.service.serving.UpdateAppRequest` class.
 * Removed `workspaceClient.cspEnablement()` service.
 * Removed `workspaceClient.esmEnablement()` service.
 * Added `workspaceClient.complianceSecurityProfile()` service.
 * Added `workspaceClient.enhancedSecurityMonitoring()` service.
 * Removed `com.databricks.sdk.service.settings.CspEnablement` class.
 * Removed `com.databricks.sdk.service.settings.CspEnablementSetting` class.
 * Removed `com.databricks.sdk.service.settings.EsmEnablement` class.
 * Removed `com.databricks.sdk.service.settings.EsmEnablementSetting` class.
 * Removed `com.databricks.sdk.service.settings.GetCspEnablementSettingRequest` class.
 * Removed `com.databricks.sdk.service.settings.GetEsmEnablementSettingRequest` class.
 * Removed `com.databricks.sdk.service.settings.UpdateCspEnablementSettingRequest` class.
 * Removed `com.databricks.sdk.service.settings.UpdateEsmEnablementSettingRequest` class.
 * Added `com.databricks.sdk.service.settings.ComplianceSecurityProfile` class.
 * Added `com.databricks.sdk.service.settings.ComplianceSecurityProfileSetting` class.
 * Added `com.databricks.sdk.service.settings.EnhancedSecurityMonitoring` class.
 * Added `com.databricks.sdk.service.settings.EnhancedSecurityMonitoringSetting` class.
 * Added `com.databricks.sdk.service.settings.GetComplianceSecurityProfileSettingRequest` class.
 * Added `com.databricks.sdk.service.settings.GetEnhancedSecurityMonitoringSettingRequest` class.
 * Added `com.databricks.sdk.service.settings.UpdateComplianceSecurityProfileSettingRequest` class.
 * Added `com.databricks.sdk.service.settings.UpdateEnhancedSecurityMonitoringSettingRequest` class.
 * Added `tags` field for `com.databricks.sdk.service.sql.DashboardEditContent`.
 * Added `tags` field for `com.databricks.sdk.service.sql.QueryEditContent`.
 * Added `catalog` field for `com.databricks.sdk.service.sql.QueryOptions`.
 * Added `schema` field for `com.databricks.sdk.service.sql.QueryOptions`.
 * Added `tags` field for `com.databricks.sdk.service.sql.QueryPostContent`.
 * Added `query` field for `com.databricks.sdk.service.sql.Visualization`.

OpenAPI SHA: 84f9315bc9cdcf3917f764cf608255a74271ab2c, Date: 2024-05-06
@tanmay-db tanmay-db mentioned this pull request May 15, 2024
github-merge-queue bot pushed a commit that referenced this pull request May 15, 2024
## 0.25.0

### New Features and Improvements
* Fix OIDC Endpoint Fetching in DatabricksConfig for Workspace Clients
([#277](#277)).
* Fix NullPointerException when reading error response body
([#276](#276)).
* Incorporate host in request after `authenticate()` call
([#282](#282)).
* Fix NPE in string function on DatabricksConfig
([#285](#285)).

### Documentation Changes
* Add instructions for building a shaded JAR
([#284](#284)).

### API Changes:
* Added `ingestionDefinition` field for
`com.databricks.sdk.service.pipelines.CreatePipeline`,
`com.databricks.sdk.service.pipelines.EditPipeline` and
`com.databricks.sdk.service.pipelines.PipelineSpec`
* Changed `create()` method for `workspaceClient.apps()` service . New
request type is `com.databricks.sdk.service.serving.CreateAppRequest`
class.
* Changed `create()` method for `workspaceClient.apps()` service to
return `com.databricks.sdk.service.serving.App` class.
* Removed `deleteApp()`, `getApp()`, `getApps()` and `getEvents()`
method for `workspaceClient.apps()` service.
* Added `createDeployment()`, `delete()`, `get()`, `getDeployment()`,
`getEnvironment()`, `list()`, `listDeployments()`, `stop()`, and
`update()` methods for `workspaceClient.apps()` service.
* Added `getOpenApi()` method for `workspaceClient.servingEndpoints()`
service.
* Changed `apps` field for
`com.databricks.sdk.service.serving.ListAppsResponse` to
`com.databricks.sdk.service.serving.AppList` class.
* Added `routeOptimized` field for
`com.databricks.sdk.service.serving.CreateServingEndpoint`.
* Added `endpointUrl` field for
`com.databricks.sdk.service.serving.ServingEndpointDetailed`.
* Added `routeOptimized` field for
`com.databricks.sdk.service.serving.ServingEndpointDetailed`.
* Added `tags` field for
`com.databricks.sdk.service.sql.DashboardEditContent`.
`com.databricks.sdk.service.sql.QueryEditContent` and
`com.databricks.sdk.service.sql.QueryPostContent`.
* Added `catalog` field for
`com.databricks.sdk.service.sql.QueryOptions`.
* Added `schema` field for
`com.databricks.sdk.service.sql.QueryOptions`.
* Added `query` field for
`com.databricks.sdk.service.sql.Visualization`.
 * Added `com.databricks.sdk.service.pipelines.IngestionConfig` class.
* Added
`com.databricks.sdk.service.pipelines.ManagedIngestionPipelineDefinition`
class.
 * Added `com.databricks.sdk.service.pipelines.SchemaSpec` class.
 * Added `com.databricks.sdk.service.pipelines.TableSpec` class.
 * Added `com.databricks.sdk.service.serving.App` class.
 * Added `com.databricks.sdk.service.serving.AppDeployment` class.
 * Added `com.databricks.sdk.service.serving.AppDeploymentState` class.
 * Added `com.databricks.sdk.service.serving.AppDeploymentStatus` class.
 * Added `com.databricks.sdk.service.serving.AppEnvironment` class.
 * Added `com.databricks.sdk.service.serving.AppState` class.
 * Added `com.databricks.sdk.service.serving.AppStatus` class.
* Added `com.databricks.sdk.service.serving.CreateAppDeploymentRequest`
class.
 * Added `com.databricks.sdk.service.serving.CreateAppRequest` class.
 * Added `com.databricks.sdk.service.serving.EnvVariable` class.
* Added `com.databricks.sdk.service.serving.GetAppDeploymentRequest`
class.
* Added `com.databricks.sdk.service.serving.GetAppEnvironmentRequest`
class.
 * Added `com.databricks.sdk.service.serving.GetOpenApiRequest` class.
 * Added `Object` class.
* Added `com.databricks.sdk.service.serving.ListAppDeploymentsRequest`
class.
* Added `com.databricks.sdk.service.serving.ListAppDeploymentsResponse`
class.
 * Added `com.databricks.sdk.service.serving.ListAppsRequest` class.
 * Added `com.databricks.sdk.service.serving.StopAppRequest` class.
 * Added `Object` class.
 * Added `com.databricks.sdk.service.serving.UpdateAppRequest` class.
* Added `com.databricks.sdk.service.settings.ComplianceSecurityProfile`
class.
* Added
`com.databricks.sdk.service.settings.ComplianceSecurityProfileSetting`
class.
* Added `com.databricks.sdk.service.settings.EnhancedSecurityMonitoring`
class.
* Added
`com.databricks.sdk.service.settings.EnhancedSecurityMonitoringSetting`
class.
* Added
`com.databricks.sdk.service.settings.GetComplianceSecurityProfileSettingRequest`
class.
* Added
`com.databricks.sdk.service.settings.GetEnhancedSecurityMonitoringSettingRequest`
class.
* Added
`com.databricks.sdk.service.settings.UpdateComplianceSecurityProfileSettingRequest`
class.
* Added
`com.databricks.sdk.service.settings.UpdateEnhancedSecurityMonitoringSettingRequest`
class.
 * Added `workspaceClient.complianceSecurityProfile()` service.
 * Added `workspaceClient.enhancedSecurityMonitoring()` service.
 * Removed `workspaceClient.cspEnablement()` service.
 * Removed `workspaceClient.esmEnablement()` service.
 * Removed `com.databricks.sdk.service.serving.AppEvents` class.
 * Removed `com.databricks.sdk.service.serving.AppManifest` class.
 * Removed `com.databricks.sdk.service.serving.AppServiceStatus` class.
 * Removed `com.databricks.sdk.service.serving.DeleteAppResponse` class.
 * Removed `com.databricks.sdk.service.serving.DeployAppRequest` class.
 * Removed `com.databricks.sdk.service.serving.DeploymentStatus` class.
* Removed `com.databricks.sdk.service.serving.DeploymentStatusState`
class.
* Removed
`com.databricks.sdk.service.serving.GetAppDeploymentStatusRequest`
class.
 * Removed `com.databricks.sdk.service.serving.GetAppResponse` class.
 * Removed `com.databricks.sdk.service.serving.GetEventsRequest` class.
* Removed `com.databricks.sdk.service.serving.ListAppEventsResponse`
class.
 * Removed `com.databricks.sdk.service.settings.CspEnablement` class.
* Removed `com.databricks.sdk.service.settings.CspEnablementSetting`
class.
 * Removed `com.databricks.sdk.service.settings.EsmEnablement` class.
* Removed `com.databricks.sdk.service.settings.EsmEnablementSetting`
class.
* Removed
`com.databricks.sdk.service.settings.GetCspEnablementSettingRequest`
class.
* Removed
`com.databricks.sdk.service.settings.GetEsmEnablementSettingRequest`
class.
* Removed
`com.databricks.sdk.service.settings.UpdateCspEnablementSettingRequest`
class.
* Removed
`com.databricks.sdk.service.settings.UpdateEsmEnablementSettingRequest`
class.

OpenAPI SHA: 84f9315bc9cdcf3917f764cf608255a74271ab2c, Date: 2024-05-06

### Internal Changes
* Update SDK to OpenAPI spec
([#280](#280)).
* Remove unused script from repository root
([#281](#281)).
* Add missing return
([#283](#283)).
* Fix test that was picking up configuration from the environment
([#287](#287)).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants