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: data-items underscore properties #2323

Merged
merged 6 commits into from
Jun 4, 2024
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
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
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ import { AppDataService } from "../data/app-data.service";
import { MockAppDataService } from "../data/app-data.service.spec";

const TEST_DATA_ROWS = [
{ id: "id1", number: 1, string: "hello", boolean: true },
{ id: "id1", number: 1, string: "hello", boolean: true, _meta_field: { test: "hello" } },
{ id: "id2", number: 2, string: "goodbye", boolean: false },
{ id: "id0", number: 3, string: "goodbye", boolean: false },
];
Expand Down Expand Up @@ -144,6 +144,21 @@ describe("DynamicDataService", () => {
expect(data[1].string).toEqual("sets an item correctly for a given _index");
});

it("supports reading data with protected fields", async () => {
const obs = await service.query$("data_list", "test_flow");
const data = await firstValueFrom(obs);
expect(data[0]["_meta_field"]).toEqual({ test: "hello" });
});
it("ignores writes to protected fields", async () => {
await service.setItem({
context: SET_ITEM_CONTEXT,
writeableProps: { _meta_field: "updated", string: "updated" },
});
const obs = await service.query$("data_list", "test_flow");
const data = await firstValueFrom(obs);
expect(data[0]["string"]).toEqual("updated");
expect(data[0]["_meta_field"]).toEqual({ test: "hello" });
});
it("adds metadata (row_index) to docs", async () => {
const obs = await service.query$<any>("data_list", "test_flow");
const data = await firstValueFrom(obs);
Expand Down
43 changes: 36 additions & 7 deletions src/app/shared/services/dynamic-data/dynamic-data.service.ts
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ import { ReactiveMemoryAdapater, REACTIVE_SCHEMA_BASE } from "./adapters/reactiv
import { TemplateActionRegistry } from "../../components/template/services/instance/template-action.registry";
import { TopLevelProperty } from "rxdb/dist/types/types";

type IDocWithID = { [key: string]: any; id: string };
type IDocWithMeta = { id: string; APP_META?: Record<string, any> };
Copy link
Collaborator

Choose a reason for hiding this comment

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

Don't we want to keep [key: string]: any to capture the fact that the doc will have other fields?

Copy link
Member Author

Choose a reason for hiding this comment

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

I think it can be argued either way - my slight preference for not emphasising the generic fields is loss of type-safety when trying to refer to the doc in code. e.g. if we changed APP_META to be something else like APP_PROTECTED typescript won't catch the error in other components that refer to the code (as the previous APP_META field would still be allowed). So here the typescript is less trying to accurately represent the data, more representing the subset of data that will be accessed directly within code. If we later want to impose more fields for certain types of datalists we can just extend the base class to keep explicit as required.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Ah yeh, that makes sense. I was only seeing the typescript's purpose as a way of accurately representing the data, but I can see how it's more useful in cases like this to have the type-safety that comes with just representing a subset of the data.


@Injectable({ providedIn: "root" })
/**
Expand Down Expand Up @@ -91,7 +91,7 @@ export class DynamicDataService extends AsyncServiceBase {
}

/** Watch for changes to a specific flow */
public async query$<T extends IDocWithID>(
public async query$<T extends IDocWithMeta>(
flow_type: FlowTypes.FlowType,
flow_name: string,
queryObj?: MangoQuery
Expand All @@ -110,7 +110,8 @@ export class DynamicDataService extends AsyncServiceBase {
return docs.map((doc) => {
// we need mutable json so that we can replace dynamic references as required
const data = doc.toMutableJSON();
return data as T;
// ensure any previously extracted metadata fields are repopulated
return this.populateMeta(data) as T;
});
})
);
Expand Down Expand Up @@ -206,21 +207,26 @@ export class DynamicDataService extends AsyncServiceBase {
delete this.collectionCreators[collectionName];
}

/** Retrive json sheet data and merge with any user writes */
/** Retrieve json sheet data and merge with any user writes */
private async getInitialData(flow_type: FlowTypes.FlowType, flow_name: string) {
const flowData = await this.appDataService.getSheet(flow_type, flow_name);
const writeData = this.writeCache.get(flow_type, flow_name) || {};
const writeDataArray: IDocWithID[] = Object.entries(writeData).map(([id, v]) => ({ ...v, id }));
const writeDataArray: IDocWithMeta[] = Object.entries(writeData).map(([id, v]) => ({
...v,
id,
}));
const mergedData = this.mergeData(flowData?.rows, writeDataArray);
return mergedData;
// HACK - rxdb can't write any fields prefixed with `_` so extract all to top-level APP_META key
const cleaned = mergedData.map((el) => this.extractMeta(el));
return cleaned;
}

/** When working with rxdb collections only alphanumeric lower case names allowed */
private normaliseCollectionName(flow_type: FlowTypes.FlowType, flow_name: string) {
return `${flow_type}${flow_name}`.toLowerCase().replace(/[^a-z0-9]/g, "");
}

private mergeData<T extends IDocWithID>(flowData: T[] = [], dbData: T[] = []) {
private mergeData<T extends IDocWithMeta>(flowData: T[] = [], dbData: T[] = []) {
const flowHashmap = arrayToHashmap(flowData, "id");
const dbDataHashmap = arrayToHashmap(dbData, "id");
const merged = deepMergeObjects(flowHashmap, dbDataHashmap);
Expand Down Expand Up @@ -285,6 +291,29 @@ export class DynamicDataService extends AsyncServiceBase {
console.warn(`[SET ITEM] - No item ${_id ? "with ID " + _id : "at index " + _index}`);
}
}

/**
* Iterate over a document's key-value pairs and populate any properties starting with
* an underscore to a single top-level APP_META property
*/
private extractMeta(doc: IDocWithMeta) {
const APP_META: Record<string, any> = {};
for (const [key, value] of Object.entries(doc)) {
if (key.startsWith("_")) {
APP_META[key] = value;
delete doc[key];
}
}
if (Object.keys(APP_META).length > 0) {
doc.APP_META = APP_META;
}
return doc;
}
/** Populate any previously extracted APP_META properties back to document */
private populateMeta(doc: IDocWithMeta) {
const { APP_META, ...data } = doc;
return { ...data, ...APP_META };
}
}

/** the context for evaluating the target item to be updated, provided by the data-items component */
Expand Down
Loading