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

chore: Move Maps API Key to database #20771

Merged
merged 51 commits into from
Jul 24, 2023
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
51 commits
Select commit Hold shift + click to select a range
00cb432
chore: Set maps key without restart
sharat87 Feb 20, 2023
0c9513f
Remove maps key from /admin/env
sharat87 Feb 20, 2023
b114e6e
Use component-page for Maps settings
sharat87 Feb 21, 2023
92ce9c9
Remove `needsRestart` changes, not needed
sharat87 Feb 21, 2023
4a2cbf3
Restore app/server/appsmith-server/src/main/java/com/appsmith/server/…
sharat87 Feb 21, 2023
69af936
Remove useless boolean response
sharat87 Feb 21, 2023
9b7a9f2
Merge remote-tracking branch 'origin/chore/maps-key-without-restart' …
sharat87 Feb 21, 2023
43181b3
Merge branch 'release' into chore/maps-key-without-restart
sharat87 Feb 21, 2023
1b8c575
Add tests for maps key
sharat87 Feb 22, 2023
c7ca322
Merge branch 'release' into chore/maps-key-without-restart
sharat87 Feb 22, 2023
4dd2da0
Merge branch 'release' into chore/maps-key-without-restart
sharat87 Feb 24, 2023
faefe7c
Fix maps input width
sharat87 Feb 25, 2023
ddb544c
Fix some test keys
sharat87 Feb 25, 2023
8d0ed94
Fix font weight
sharat87 Feb 25, 2023
94fc749
Fix empty response from PUT /admin/env
sharat87 Feb 26, 2023
32aab44
Merge branch 'release' into chore/maps-key-without-restart
sharat87 Apr 25, 2023
6cbc1c2
Fix type imports
sharat87 Apr 25, 2023
25b2302
Merge branch 'release' into chore/maps-key-without-restart
sharat87 May 1, 2023
8b700f7
fix: test failure
sharat87 May 2, 2023
dd1eade
Merge branch 'release' into chore/maps-key-without-restart
sharat87 May 4, 2023
4ac21ed
Merge branch 'release' into chore/maps-key-without-restart
sharat87 Jun 8, 2023
3444b7f
Fix compile error
sharat87 Jun 8, 2023
6e78a8e
Fix UI
sharat87 Jun 8, 2023
63f0524
Undo unneeded exports
sharat87 Jun 8, 2023
0383eb2
Merge branch 'release' into chore/maps-key-without-restart
sharat87 Jun 12, 2023
77f79cf
Fix missing test id for tests
sharat87 Jun 12, 2023
1ad9e96
Merge branch 'release' into chore/maps-key-without-restart
sharat87 Jul 1, 2023
97be740
Undo changes to env API for saving maps key
sharat87 Jul 1, 2023
1147d78
Use tenant API to save changes in Maps key
sharat87 Jul 1, 2023
49dfa6b
Fix compile error in backend test
sharat87 Jul 1, 2023
ff62866
Add migration to comment out maps key env variable
sharat87 Jul 1, 2023
8ad43c2
Copy fix
sharat87 Jul 1, 2023
e3074f2
Lint fix
sharat87 Jul 1, 2023
57bff18
Back to googleMaps.ts and fix server tests
sharat87 Jul 2, 2023
ca83278
Revert unneeded changes
sharat87 Jul 2, 2023
42b3c44
Remove maps env variable from initial templates
sharat87 Jul 2, 2023
70fa722
Remove unused TestUtils
sharat87 Jul 2, 2023
abfc787
Fix lint
sharat87 Jul 2, 2023
3898ecb
Show Save button on Maps settings page
sharat87 Jul 2, 2023
bd452e8
Fix test compile
sharat87 Jul 2, 2023
e84d588
Respond with error when no access to update tenant
sharat87 Jul 3, 2023
3b92a10
Merge branch 'release' into chore/maps-key-without-restart
sharat87 Jul 17, 2023
f46b227
Fix formatting
sharat87 Jul 17, 2023
a1a0500
Fix tests
sharat87 Jul 17, 2023
c48c991
Fix form login setting fail
sharat87 Jul 18, 2023
8002d28
updating only tenant cnfig condition
ankitakinger Jul 18, 2023
1cb3681
Merge branch 'release' of https://github.com/appsmithorg/appsmith int…
ankitakinger Jul 18, 2023
c6a0688
Merge branch 'release' into chore/maps-key-without-restart
sharat87 Jul 20, 2023
70e7236
Introducing "Save & Refresh"!
sharat87 Jul 21, 2023
d2a5404
Merge release
sharat87 Jul 21, 2023
4d05a25
Allow null in `getClientPertinentTenant`, used in EE
sharat87 Jul 21, 2023
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Next Next commit
chore: Set maps key without restart
  • Loading branch information
sharat87 committed Feb 20, 2023
commit 00cb4320028e5ddacb47f8d6975f35641d8607ca
2 changes: 2 additions & 0 deletions app/client/src/ce/pages/AdminSettings/config/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -94,6 +94,7 @@ export interface Category {
isConnected?: boolean;
children?: Category[];
icon?: string;
needsRestart?: boolean;
}

export const SettingCategories = {
Expand Down Expand Up @@ -129,4 +130,5 @@ export type AdminConfigType = {
isConnected?: boolean;
icon?: string;
needsUpgrade?: boolean;
needsRestart?: boolean;
};
5 changes: 3 additions & 2 deletions app/client/src/pages/Settings/SaveSettings.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -41,14 +41,15 @@ const SettingsButtonWrapper = styled.div`

type SaveAdminSettingsProps = {
isSaving?: boolean;
needsRestart: boolean;
onSave?: () => void;
onClear?: () => void;
settings: Record<string, string>;
valid: boolean;
};

const saveAdminSettings = (props: SaveAdminSettingsProps) => {
const { isSaving, onClear, onSave, settings, valid } = props;
const { isSaving, needsRestart, onClear, onSave, settings, valid } = props;

return (
<SettingsButtonWrapper>
Expand All @@ -59,7 +60,7 @@ const saveAdminSettings = (props: SaveAdminSettingsProps) => {
isLoading={isSaving}
onClick={onSave}
tag="button"
text={createMessage(() => "Save & Restart")}
text={createMessage(() => (needsRestart ? "Save & Restart" : "Save"))}
/>
<StyledClearButton
category={Category.secondary}
Expand Down
3 changes: 2 additions & 1 deletion app/client/src/pages/Settings/SettingsForm.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -87,7 +87,7 @@ export function SettingsForm(
AnalyticsUtil.logEvent("ADMIN_SETTINGS_SAVE", {
method: pageTitle,
});
dispatch(saveSettings(props.settings));
dispatch(saveSettings(props.settings, details?.needsRestart ?? true));
} else {
saveBlocked();
}
Expand Down Expand Up @@ -201,6 +201,7 @@ export function SettingsForm(
{isSavable && (
<SaveAdminSettings
isSaving={props.isSaving}
needsRestart={details?.needsRestart ?? true}
onClear={onClear}
onSave={onSave}
settings={props.settings}
Expand Down
1 change: 1 addition & 0 deletions app/client/src/pages/Settings/config/ConfigFactory.ts
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,7 @@ export class ConfigFactory {
title: config.title,
slug: config.type,
subText: config.subText,
needsRestart: config.needsRestart,
children: config?.children?.map((child) =>
ConfigFactory.getCategory(child),
),
Expand Down
1 change: 1 addition & 0 deletions app/client/src/pages/Settings/config/googleMaps.ts
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ export const config: AdminConfigType = {
controlType: SettingTypes.GROUP,
title: "Google Maps",
canSave: true,
needsRestart: false,
settings: [
{
id: "APPSMITH_GOOGLE_MAPS_READ_MORE",
Expand Down
Original file line number Diff line number Diff line change
@@ -1,7 +1,6 @@
package com.appsmith.server.controllers.ce;

import com.appsmith.server.constants.Url;
import com.appsmith.server.dtos.EnvChangesResponseDTO;
import com.appsmith.server.dtos.ResponseDTO;
import com.appsmith.server.dtos.TestEmailConfigRequestDTO;
import com.appsmith.server.solutions.EnvManager;
Expand Down Expand Up @@ -42,22 +41,22 @@ public Mono<Void> download(ServerWebExchange exchange) {

@Deprecated
@PutMapping(value = "/env", consumes = {MediaType.APPLICATION_JSON_VALUE})
public Mono<ResponseDTO<EnvChangesResponseDTO>> saveEnvChangesJSON(
public Mono<ResponseDTO<Void>> saveEnvChangesJSON(
@Valid @RequestBody Map<String, String> changes
) {
log.debug("Applying env updates {}", changes.keySet());
return envManager.applyChanges(changes)
.map(res -> new ResponseDTO<>(HttpStatus.OK.value(), res, null));
.map(res -> new ResponseDTO<>(HttpStatus.OK.value(), null, null));
}

@PutMapping(value = "/env", consumes = {MediaType.MULTIPART_FORM_DATA_VALUE})
public Mono<ResponseDTO<EnvChangesResponseDTO>> saveEnvChangesMultipartFormData(
public Mono<ResponseDTO<Void>> saveEnvChangesMultipartFormData(
ServerWebExchange exchange
) {
log.debug("Applying env updates from form data");
return exchange.getMultipartData()
.flatMap(envManager::applyChangesFromMultipartFormData)
.map(res -> new ResponseDTO<>(HttpStatus.OK.value(), res, null));
.map(res -> new ResponseDTO<>(HttpStatus.OK.value(), null, null));
}

@PostMapping("/restart")
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,6 @@

import com.appsmith.server.constants.Url;
import com.appsmith.server.domains.Tenant;
import com.appsmith.server.domains.TenantConfiguration;
import com.appsmith.server.dtos.ResponseDTO;
import com.appsmith.server.services.TenantService;
import org.springframework.http.HttpStatus;
Expand All @@ -23,7 +22,7 @@ public TenantControllerCE(TenantService service) {

/**
* This API returns the tenant configuration for any user (anonymous or logged in). The configurations are set
* in {@link com.appsmith.server.controllers.ce.InstanceAdminControllerCE#saveEnvChanges(Map<String,String>)}
* in {@link com.appsmith.server.controllers.ce.InstanceAdminControllerCE#saveEnvChangesJSON(Map<String,String>)}
* <p>
* The update and retrieval are in different controllers because it would have been weird to fetch the configurations
* from the InstanceAdminController
Expand Down
Original file line number Diff line number Diff line change
@@ -1,10 +1,17 @@
package com.appsmith.server.domains.ce;

import com.appsmith.server.domains.TenantConfiguration;
import com.fasterxml.jackson.annotation.JsonProperty;
import lombok.Data;

@Data
public class TenantConfigurationCE {

@JsonProperty("APPSMITH_GOOGLE_MAPS_API_KEY")
private String googleMapsKey;

public void copyNonSensitiveValues(TenantConfiguration source) {
googleMapsKey = source.getGoogleMapsKey();
}

}

This file was deleted.

Original file line number Diff line number Diff line change
Expand Up @@ -14,9 +14,8 @@ public interface TenantServiceCE extends CrudService<Tenant, String> {

Mono<Tenant> findById(String tenantId, AclPermission permission);

/*
* For now, returning an empty tenantConfiguration object in this class. Will enhance this function once we
* start saving other pertinent environment variables in the tenant collection
*/
Mono<Tenant> getDefaultTenant();

Mono<Tenant> getTenantConfiguration();

}
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@

import com.appsmith.external.helpers.AppsmithBeanUtils;
import com.appsmith.server.acl.AclPermission;
import com.appsmith.server.constants.EnvVariables;
import com.appsmith.server.constants.FieldName;
import com.appsmith.server.domains.Tenant;
import com.appsmith.server.domains.TenantConfiguration;
Expand Down Expand Up @@ -75,23 +76,40 @@ public Mono<Tenant> findById(String tenantId, AclPermission permission) {
.switchIfEmpty(Mono.error(new AppsmithException(AppsmithError.NO_RESOURCE_FOUND, "tenantId", tenantId)));
}

/*
* For now, returning just the instance-id, with an empty tenantConfiguration object in this class. Will enhance
* this function once we start saving other pertinent environment variables in the tenant collection.
*/
@Override
public Mono<Tenant> getTenantConfiguration() {
return configService.getInstanceId()
.map(instanceId -> {
final Tenant tenant = new Tenant();
tenant.setInstanceId(instanceId);

final TenantConfiguration config = new TenantConfiguration();
tenant.setTenantConfiguration(config);
public Mono<Tenant> getDefaultTenant() {
// Get the default tenant object from the DB and then populate the relevant user permissions in that
// We are doing this differently because `findBySlug` is a Mongo JPA query and not a custom Appsmith query
return repository.findBySlug(FieldName.DEFAULT)
.flatMap(tenant -> repository.setUserPermissionsInObject(tenant).thenReturn(tenant));
}

config.setGoogleMapsKey(System.getenv("APPSMITH_GOOGLE_MAPS_API_KEY"));
@Override
public Mono<Tenant> getTenantConfiguration() {
return Mono.zip(
getDefaultTenant(),
configService.getInstanceId()
)
.map(tuple -> {
final Tenant dbTenant = tuple.getT1();
final String instanceId = tuple.getT2();

final Tenant tenantForClient = new Tenant();
tenantForClient.setInstanceId(instanceId);

final TenantConfiguration configForClient = new TenantConfiguration();
tenantForClient.setTenantConfiguration(configForClient);

// Only copy the values that are pertinent to the client
configForClient.copyNonSensitiveValues(dbTenant.getTenantConfiguration());
tenantForClient.setUserPermissions(dbTenant.getUserPermissions());

if (StringUtils.isEmpty(configForClient.getGoogleMapsKey())) {
configForClient.setGoogleMapsKey(System.getenv(EnvVariables.APPSMITH_GOOGLE_MAPS_API_KEY.name()));
}

return tenant;
return tenantForClient;
});
}

}
Original file line number Diff line number Diff line change
@@ -1,7 +1,6 @@
package com.appsmith.server.solutions.ce;

import com.appsmith.server.domains.User;
import com.appsmith.server.dtos.EnvChangesResponseDTO;
import com.appsmith.server.dtos.TestEmailConfigRequestDTO;
import org.springframework.http.codec.multipart.Part;
import org.springframework.util.MultiValueMap;
Expand All @@ -16,9 +15,9 @@ public interface EnvManagerCE {

List<String> transformEnvContent(String envContent, Map<String, String> changes);

Mono<EnvChangesResponseDTO> applyChanges(Map<String, String> changes);
Mono<Boolean> applyChanges(Map<String, String> changes);

Mono<EnvChangesResponseDTO> applyChangesFromMultipartFormData(MultiValueMap<String, Part> formData);
Mono<Boolean> applyChangesFromMultipartFormData(MultiValueMap<String, Part> formData);

void setAnalyticsEventAction(Map<String, Object> properties, String newVariable, String originalVariable, String authEnv);

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@
import com.appsmith.server.domains.Tenant;
import com.appsmith.server.domains.TenantConfiguration;
import com.appsmith.server.domains.User;
import com.appsmith.server.dtos.EnvChangesResponseDTO;
import com.appsmith.server.domains.ce.TenantConfigurationCE;
import com.appsmith.server.dtos.TestEmailConfigRequestDTO;
import com.appsmith.server.exceptions.AppsmithError;
import com.appsmith.server.exceptions.AppsmithException;
Expand Down Expand Up @@ -296,12 +296,12 @@ private Mono<Void> validateChanges(User user, Map<String, String> changes) {
* @return
*/
private Set<String> allowedTenantConfiguration() {
Field[] fields = TenantConfiguration.class.getDeclaredFields();
return Arrays.stream(fields)
return getTenantConfigurationFields().stream()
.map(field -> {
JsonProperty jsonProperty = field.getDeclaredAnnotation(JsonProperty.class);
return jsonProperty == null ? field.getName() : jsonProperty.value();
}).collect(Collectors.toSet());
})
.collect(Collectors.toSet());
}

/**
Expand All @@ -313,8 +313,7 @@ private Set<String> allowedTenantConfiguration() {
* @param value
*/
private void setConfigurationByKey(TenantConfiguration tenantConfiguration, String key, String value) {
Field[] fields = tenantConfiguration.getClass().getDeclaredFields();
for (Field field : fields) {
for (Field field : getTenantConfigurationFields()) {
JsonProperty jsonProperty = field.getDeclaredAnnotation(JsonProperty.class);
if (jsonProperty != null && jsonProperty.value().equals(key)) {
try {
Expand All @@ -328,6 +327,12 @@ private void setConfigurationByKey(TenantConfiguration tenantConfiguration, Stri
}
}

@NotNull
private static List<Field> getTenantConfigurationFields() {
final List<Field> fields = Arrays.asList(TenantConfigurationCE.class.getDeclaredFields());
fields.addAll(Arrays.asList(TenantConfiguration.class.getDeclaredFields()));
return fields;
}

private Mono<Tenant> updateTenantConfiguration(String tenantId, Map<String, String> changes) {
TenantConfiguration tenantConfiguration = new TenantConfiguration();
Expand All @@ -344,7 +349,7 @@ private Mono<Tenant> updateTenantConfiguration(String tenantId, Map<String, Stri
}

@Override
public Mono<EnvChangesResponseDTO> applyChanges(Map<String, String> changes) {
public Mono<Boolean> applyChanges(Map<String, String> changes) {
// This flow is pertinent for any variables that need to change in the .env file or be saved in the tenant configuration
return verifyCurrentUserIsSuper().
flatMap(user -> validateChanges(user, changes).thenReturn(user))
Expand All @@ -368,13 +373,17 @@ public Mono<EnvChangesResponseDTO> applyChanges(Map<String, String> changes) {
envFileChanges.remove(key);
}
}
final List<String> changedContent = transformEnvContent(originalContent, envFileChanges);

try {
Files.write(envFilePath, changedContent);
} catch (IOException e) {
log.error("Unable to write to env file " + envFilePath, e);
return Mono.error(e);
if (!envFileChanges.isEmpty()) {
// If there's only tenant configuration changes, env file doesn't have to be written.
final List<String> changedContent = transformEnvContent(originalContent, envFileChanges);

try {
Files.write(envFilePath, changedContent);
} catch (IOException e) {
log.error("Unable to write to env file " + envFilePath, e);
return Mono.error(e);
}
}

// For configuration variables, save the variables to the config collection instead of .env file
Expand Down Expand Up @@ -452,12 +461,12 @@ public Mono<EnvChangesResponseDTO> applyChanges(Map<String, String> changes) {
commonConfig.setTelemetryDisabled("true".equals(changesCopy.remove(APPSMITH_DISABLE_TELEMETRY.name())));
}

return dependentTasks.thenReturn(new EnvChangesResponseDTO(true));
return dependentTasks.thenReturn(true);
});
}

@Override
public Mono<EnvChangesResponseDTO> applyChangesFromMultipartFormData(MultiValueMap<String, Part> formData) {
public Mono<Boolean> applyChangesFromMultipartFormData(MultiValueMap<String, Part> formData) {
return Flux.fromIterable(formData.entrySet())
.flatMap(entry -> {
final String key = entry.getKey();
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,29 @@
package com.appsmith.server.services.ce;

import com.appsmith.server.services.TenantService;
import org.junit.jupiter.api.Test;
import org.junit.jupiter.api.extension.ExtendWith;
import org.springframework.beans.factory.annotation.Autowired;
import org.springframework.boot.test.context.SpringBootTest;
import org.springframework.test.context.junit.jupiter.SpringExtension;
import reactor.test.StepVerifier;

import static org.assertj.core.api.Assertions.assertThat;

@SpringBootTest
@ExtendWith(SpringExtension.class)
class TenantServiceCETest {

@Autowired
TenantService tenantService;

@Test
void ensureMapsKey() {
StepVerifier.create(tenantService.getTenantConfiguration())
.assertNext(tenant -> {
assertThat(tenant.getTenantConfiguration().getGoogleMapsKey()).isEqualTo("abcd");
})
.verifyComplete();
}

}