From 99f2d1a686fe349d85734b9657441fe1c0384ad8 Mon Sep 17 00:00:00 2001 From: Richard Cox Date: Wed, 16 Jan 2019 11:57:19 +0000 Subject: [PATCH 1/2] Fix null exception after creating a space in an new org - We previously nuked the org's space collection when creating or deleting an org - Validation was correctly then going to refetch this list, however waitForEntity is broken (see #2327) and emits an invalid action - Later code takes invalid action an tries to access space - Fix, aside from #2327, is yodate org space collection after space creation instead of nuking and waiting for validation to refetch Other fixes - Found sub leak in edit org stepper - endpoint service allApps is subbed to in many places, ensure we share --- .../edit-organization-step.component.ts | 3 +- .../cloud-foundry-endpoint.service.ts | 4 +- .../reducers/organization-space.reducer.ts | 42 ++++++++++++------- 3 files changed, 33 insertions(+), 16 deletions(-) diff --git a/src/frontend/app/features/cloud-foundry/edit-organization/edit-organization-step/edit-organization-step.component.ts b/src/frontend/app/features/cloud-foundry/edit-organization/edit-organization-step/edit-organization-step.component.ts index 2d081291f0..73320179ea 100644 --- a/src/frontend/app/features/cloud-foundry/edit-organization/edit-organization-step/edit-organization-step.component.ts +++ b/src/frontend/app/features/cloud-foundry/edit-organization/edit-organization-step/edit-organization-step.component.ts @@ -5,6 +5,7 @@ import { Observable, Subscription } from 'rxjs'; import { filter, map, take, tap } from 'rxjs/operators'; import { IOrganization } from '../../../../core/cf-api.types'; +import { safeUnsubscribe } from '../../../../core/utils.service'; import { StepOnNextFunction } from '../../../../shared/components/stepper/step/step.component'; import { PaginationMonitorFactory } from '../../../../shared/monitors/pagination-monitor.factory'; import { UpdateOrganization } from '../../../../store/actions/organization.actions'; @@ -126,6 +127,6 @@ export class EditOrganizationStepComponent implements OnInit, OnDestroy { } ngOnDestroy(): void { - this.fetchOrgsSub.unsubscribe(); + safeUnsubscribe(this.fetchOrgsSub, this.orgSubscription); } } diff --git a/src/frontend/app/features/cloud-foundry/services/cloud-foundry-endpoint.service.ts b/src/frontend/app/features/cloud-foundry/services/cloud-foundry-endpoint.service.ts index 5956411c13..a035e6bcaf 100644 --- a/src/frontend/app/features/cloud-foundry/services/cloud-foundry-endpoint.service.ts +++ b/src/frontend/app/features/cloud-foundry/services/cloud-foundry-endpoint.service.ts @@ -182,7 +182,9 @@ export class CloudFoundryEndpointService { this.allApps$ = pagObs.entities$.pipe(// Ensure we sub to entities to kick off fetch process switchMap(() => pagObs.pagination$), filter(pagination => !!pagination && !!pagination.pageRequests && !!pagination.pageRequests[1] && !pagination.pageRequests[1].busy), - switchMap(pagination => pagination.maxedResults ? observableOf(null) : pagObs.entities$) + switchMap(pagination => pagination.maxedResults ? observableOf(null) : pagObs.entities$), + publishReplay(1), + refCount() ); this.loadingApps$ = pagObs.entities$.pipe(// Ensure we sub to entities to kick off fetch process diff --git a/src/frontend/app/store/reducers/organization-space.reducer.ts b/src/frontend/app/store/reducers/organization-space.reducer.ts index 3a0e42d9ac..e961d52f7f 100644 --- a/src/frontend/app/store/reducers/organization-space.reducer.ts +++ b/src/frontend/app/store/reducers/organization-space.reducer.ts @@ -1,22 +1,33 @@ import { IOrganization, ISpace } from '../../core/cf-api.types'; -import { BaseSpaceAction, CREATE_SPACE_SUCCESS, DELETE_SPACE_SUCCESS } from '../actions/space.actions'; -import { APIResource } from '../types/api.types'; +import { + BaseSpaceAction, + CREATE_SPACE_SUCCESS, + CreateSpace, + DELETE_SPACE_SUCCESS, + DeleteSpace, +} from '../actions/space.actions'; +import { spaceSchemaKey } from '../helpers/entity-factory'; +import { APIResource, NormalizedResponse } from '../types/api.types'; import { APISuccessOrFailedAction } from '../types/request.types'; -// Note - This reducer will be updated when we address general entity relation deletion +// Note - This reducer will be updated when we address general deletion of entities within inline lists (not paginated lists) export function updateOrganizationSpaceReducer() { return function (state: APIResource, action: APISuccessOrFailedAction) { switch (action.type) { case DELETE_SPACE_SUCCESS: + const deleteSpaceAction: DeleteSpace = action.apiAction as DeleteSpace; + return updateOrgSpaces(state, deleteSpaceAction.orgGuid, deleteSpaceAction); case CREATE_SPACE_SUCCESS: - const spaceAction: BaseSpaceAction = action.apiAction as BaseSpaceAction; - return deleteOrgSpaces(state, spaceAction.orgGuid, spaceAction); + const createSpaceAction: CreateSpace = action.apiAction as CreateSpace; + const response: NormalizedResponse = action.response as NormalizedResponse; + const space = response.entities[spaceSchemaKey][response.result[0]]; + return updateOrgSpaces(state, createSpaceAction.orgGuid, createSpaceAction, space); } return state; }; } -function deleteOrgSpaces(state: APIResource, orgGuid: string, spaceAction: BaseSpaceAction) { +function updateOrgSpaces(state: APIResource, orgGuid: string, spaceAction: BaseSpaceAction, newSpace?: APIResource) { if (!orgGuid) { return state; } @@ -30,14 +41,17 @@ function deleteOrgSpaces(state: APIResource, orgGuid: string, spaceAction: BaseS orgGuids.forEach(currentGuid => { const org: APIResource = state[currentGuid]; if (currentGuid === orgGuid) { - let newSpaces: APIResource[] = null; - if (spaceAction.removeEntityOnDelete) { - const spaceIndex = org.entity.spaces.findIndex(space => { - return typeof (space) === 'string' ? space === spaceAction.guid : space.metadata.guid === spaceAction.guid; - }); - if (spaceIndex >= 0) { - newSpaces = [...org.entity.spaces]; - newSpaces.splice(spaceIndex, 1); + const newSpaces: (APIResource | string)[] = [...org.entity.spaces]; + if (newSpace) { + newSpaces.push(newSpace.metadata.guid); + } else { + if (spaceAction.removeEntityOnDelete) { + const spaceIndex = org.entity.spaces.findIndex(space => { + return typeof (space) === 'string' ? space === spaceAction.guid : space.metadata.guid === spaceAction.guid; + }); + if (spaceIndex >= 0) { + newSpaces.splice(spaceIndex, 1); + } } } const newOrg = { From 24833567182d442f5a1581a22052490538f804a7 Mon Sep 17 00:00:00 2001 From: Nathan Jones Date: Wed, 16 Jan 2019 17:21:00 +0000 Subject: [PATCH 2/2] Tidy up org spce reducer logic --- src/frontend/app/core/cf-api.types.ts | 4 +- .../reducers/organization-space.reducer.ts | 105 +++++++++++------- 2 files changed, 64 insertions(+), 45 deletions(-) diff --git a/src/frontend/app/core/cf-api.types.ts b/src/frontend/app/core/cf-api.types.ts index c3a8addf0e..c3e5d0e459 100644 --- a/src/frontend/app/core/cf-api.types.ts +++ b/src/frontend/app/core/cf-api.types.ts @@ -154,7 +154,7 @@ export interface IDeveloper { audited_spaces_url: string; } -export interface IOrganization { +export interface IOrganization[]> { name: string; billing_enabled?: boolean; quota_definition_guid?: string; @@ -176,7 +176,7 @@ export interface IOrganization { space_quota_definitions_url?: string; guid?: string; cfGuid?: string; - spaces?: APIResource[]; + spaces?: spaceT; private_domains?: APIResource[]; quota_definition?: APIResource; } diff --git a/src/frontend/app/store/reducers/organization-space.reducer.ts b/src/frontend/app/store/reducers/organization-space.reducer.ts index e961d52f7f..bceb39ad5d 100644 --- a/src/frontend/app/store/reducers/organization-space.reducer.ts +++ b/src/frontend/app/store/reducers/organization-space.reducer.ts @@ -9,62 +9,81 @@ import { import { spaceSchemaKey } from '../helpers/entity-factory'; import { APIResource, NormalizedResponse } from '../types/api.types'; import { APISuccessOrFailedAction } from '../types/request.types'; - +import { IRequestEntityTypeState } from '../app-state'; +type entityOrgType = APIResource>; // Note - This reducer will be updated when we address general deletion of entities within inline lists (not paginated lists) export function updateOrganizationSpaceReducer() { - return function (state: APIResource, action: APISuccessOrFailedAction) { + return function (state: IRequestEntityTypeState, action: APISuccessOrFailedAction) { switch (action.type) { case DELETE_SPACE_SUCCESS: const deleteSpaceAction: DeleteSpace = action.apiAction as DeleteSpace; - return updateOrgSpaces(state, deleteSpaceAction.orgGuid, deleteSpaceAction); + return removeSpaceFromOrg(state, deleteSpaceAction.orgGuid, deleteSpaceAction.guid); case CREATE_SPACE_SUCCESS: - const createSpaceAction: CreateSpace = action.apiAction as CreateSpace; - const response: NormalizedResponse = action.response as NormalizedResponse; + const createSpaceAction = action.apiAction as CreateSpace; + const response = action.response; const space = response.entities[spaceSchemaKey][response.result[0]]; - return updateOrgSpaces(state, createSpaceAction.orgGuid, createSpaceAction, space); + return addSpaceToOrg(state, createSpaceAction.orgGuid, space); } return state; }; } -function updateOrgSpaces(state: APIResource, orgGuid: string, spaceAction: BaseSpaceAction, newSpace?: APIResource) { - if (!orgGuid) { - return state; - } +function addSpaceToOrg( + state: IRequestEntityTypeState, + orgGuid: string, + newSpace: APIResource +) { + const orgToModify = getOrg(state, orgGuid); + const newSpaces = [ + ...orgToModify.entity.spaces, + newSpace.metadata.guid + ]; + const mergedOrg = applySpacesToOrg(orgToModify, newSpaces); + return { + ...state, + [orgGuid]: mergedOrg + }; +} - const orgGuids = Object.keys(state); - if (orgGuids.indexOf(orgGuid) === -1) { - return state; - } +function removeSpaceFromOrg( + state: IRequestEntityTypeState, + orgGuid: string, + spaceGuid: string +) { + const orgToModify = getOrg(state, orgGuid); + const newSpaces = orgToModify.entity.spaces.reduce((spaceIds, spaceId) => { + if (spaceId !== spaceGuid) { + spaceIds.push(spaceId); + } + return spaceIds; + }, []); + const mergedOrg = applySpacesToOrg(orgToModify, newSpaces); + return applyModifyOrgToState(state, mergedOrg); +} - const newState = {}; - orgGuids.forEach(currentGuid => { - const org: APIResource = state[currentGuid]; - if (currentGuid === orgGuid) { - const newSpaces: (APIResource | string)[] = [...org.entity.spaces]; - if (newSpace) { - newSpaces.push(newSpace.metadata.guid); - } else { - if (spaceAction.removeEntityOnDelete) { - const spaceIndex = org.entity.spaces.findIndex(space => { - return typeof (space) === 'string' ? space === spaceAction.guid : space.metadata.guid === spaceAction.guid; - }); - if (spaceIndex >= 0) { - newSpaces.splice(spaceIndex, 1); - } - } - } - const newOrg = { - ...org, - entity: { - ...org.entity, - spaces: newSpaces - } - }; - newState[currentGuid] = newOrg; - } else { - newState[currentGuid] = org; +function applySpacesToOrg(org: entityOrgType, spaces: string[]): entityOrgType { + return { + ...org, + entity: { + ...org.entity, + spaces } - }); - return newState; + }; +} + +function applyModifyOrgToState(state: IRequestEntityTypeState, org: entityOrgType) { + return { + ...state, + [org.metadata.guid]: org + }; +} + +function getOrg( + state: IRequestEntityTypeState, + orgGuid: string, +) { + const { + [orgGuid]: newOrg + } = state; + return newOrg; }