Fetch compound document #105

leoraba · 2024-10-22T12:38:19Z

Description

This PR introduces a new feature to this project - View representation of a compound document represented in a hierarchical structure of related Submitted Data, consolidating all related data for a central entity, or centric schema, while organizing both its parent and child relationships within the document. This structure will enable more efficient data access by aggregating connected data into a single view.

Here's how each relationships are represented:

Centric Schema: The central entity is the focal point of the document. All related data, including both parent and child records, is organized around this entity.
Child entities: Displayed in arrays within the centric schema document. Each child entry includes relevant information and may also contain an array of its own children allowing for a nested, hierarchical structure.
Parent entity: representing the entity to which the centric entity belongs, is stored as a single, unique field within the centric schema document.

Code changes:

Changes on existing GET data endpoints to add a query parameter view. This specify the data representation to be returned. Use list to return an unordered list of entities; compound to return a compound document nesting using scheme centric structure.
Change on register new Dictionary endpoint to include new body parameter defaultCentricEntity. This value is used to form compound documents.
New environment variables: NESTED_RECORD_PREFIX, NESTED_RECORD_SUFFIX , PARENT_RECORD_PREFIX, PARENT_RECORD_SUFFIX
Add new GET data by systemID endpoint.

leoraba · 2024-10-23T13:04:24Z

apps/server/swagger/data-api.yml

+/data/category/{categoryId}/id/{systemId}:
+  get:
+    summary: Retrieve Submitted Data Record for a System ID


This is a new Endpoint to retrieve record by it's systemID

leoraba · 2024-10-23T13:04:49Z

apps/server/swagger/data-api.yml

@@ -31,6 +31,13 @@
        schema:
          type: integer
        description: Optional query parameter to specify the number of results per page. Default value is 20
+      - name: view
+        in: query


adding view query param on GET data endpoints

I'm nervous about the performance of compound object retrieval in a list... there could be many joins that are slow to resolve. I suppose if this response is paginated then this is less of a concern.

Am I correct in assuming that the purpose of the defaultCentricEntity property you are introducing is to determine which entity to use as the root when returning data in this form?

This endpoint and others that fetch data as list or compound implements pagination. with default of 20 items per page. This will alleviate fetching large mount of data.

It's correct, the property defaultCentricEntity is used to determine thee root schema in compound view

leoraba · 2024-10-23T13:06:10Z

apps/server/swagger/dictionary-api.yml

+              defaultCentricEntity:
+                type: string
+                description: The default centric entity name


Adding new input parameter on Dictionary Registration endpoint. This is value is used to retrieve compound records

leoraba · 2024-10-23T13:07:08Z

packages/data-provider/src/utils/dictionarySchemaRelations.ts

+ * @param order Order of the structed.
+ * @returns The hierarchical tree structure.
+ */
+export const generateHierarchy = (source: SchemaDefinition[], order: OrderType): TreeNode[] => {


main function to generate hierarchy structure based on dictionary schemas.

Did you consider writing this function to return both the ascending and descending hierarchies in one call? Do you we have a use case where we want only one and not the other?

I see us using this function in 3 places, and each time we call it for both ascending and descending trees.

demariadaniel · 2024-10-28T18:06:49Z

packages/data-model/docs/README.md

+
+- `original_schema_id`: This field references the original schema version used during the initial submission of the data, allowing for a historical perspective on schema changes over time.
+
+- `submission_id`: This field links the audit record to the specific submission, creating a direct connection to the submissin responsible for this action.


... connection to the submission responsible for this action.
Missing 'o' in submission

demariadaniel · 2024-10-28T18:08:09Z

packages/data-model/docs/README.md

+- `created_at`: This timestamp records when the audit entry was created, providing a chronological context for the logged action.
+
+- `created_by`: This field indicates who performed the action, enhancing accountability by identifying the user responsible for the change.
+


These are great docs 👍

demariadaniel · 2024-10-28T18:13:09Z

packages/data-provider/src/controllers/submittedDataController.ts

@@ -10,15 +10,18 @@ import {
 	dataGetByCategoryRequestSchema,
 	dataGetByOrganizationRequestSchema,
 	dataGetByQueryRequestschema,
+	dataGetBySystemIdRequestschema,


Inconsistent casing for variables:

dataGetByCategoryRequestSchema, dataGetByOrganizationRequestSchema,
vs
dataGetByQueryRequestschema, dataGetBySystemIdRequestschema,

demariadaniel · 2024-10-28T18:21:47Z

packages/data-model/migrations/0009_add_default_centric_entity.sql

@@ -0,0 +1,2 @@
+ALTER TABLE "dictionary_categories" ALTER COLUMN "active_dictionary_id" SET NOT NULL;--> statement-breakpoint


Is --> statement-breakpoint intended to be committed?

The .sql file is auto generated by Drizzle Kit. unfortunately this comment is added

demariadaniel · 2024-10-28T18:35:24Z

packages/data-provider/src/utils/dictionarySchemaRelations.ts

+};
+
+/**
+ * Finds a matching schema name withing a nested object.


Typo within a nested object (extra 'g')

demariadaniel · 2024-10-28T19:27:49Z

packages/data-provider/src/services/submittedDataService.ts

 	const getSubmittedDataByCategory = async (
 		categoryId: number,
 		paginationOptions: PaginationOptions,
-		filterOptions: { entityName?: (string | undefined)[] },
+		filterOptions: { entityName?: (string | undefined)[]; view: ViewType },


view here could be optional right? { entityName?: (string | undefined)[]; view?: ViewType },

On the Controllers level, view is optional. For service functions, however, it is required, and we define a default view for them.

demariadaniel · 2024-10-28T19:35:19Z

packages/data-provider/src/services/submittedDataService.ts

 	const getSubmittedDataByOrganization = async (
 		categoryId: number,
 		organization: string,
 		paginationOptions: PaginationOptions,
-		filterOptions?: { sqon?: SQON; entityName?: (string | undefined)[] },
+		filterOptions: { sqon?: SQON; entityName?: (string | undefined)[]; view: ViewType },


view here is also optional

Also there's enough overlap between getSubmittedDataByCategory & getSubmittedDataByOrganization that filterOptions could be defined as a reusable variable

demariadaniel · 2024-10-28T19:41:31Z

packages/data-provider/src/services/submittedDataService.ts

+	const getSubmittedDataBySystemId = async (
+		categoryId: number,
+		systemId: string,
+		filterOptions: { view: ViewType },


Tied into the above discussion, view is not mandatory here

demariadaniel · 2024-10-28T19:52:19Z

packages/data-provider/test/utils/dictionarySchemaRelations.spec.ts

-
-		expect(result).to.eql({ sports: [] });
+
+	describe('find the hierarchycal descendant structure between schemas in the dictionary', () => {


Spelling hierarchical

That looks like a made up word lol.

demariadaniel · 2024-10-28T19:52:35Z

packages/data-provider/test/utils/dictionarySchemaRelations.spec.ts


 describe('Dictionary Schema Relations', () => {
-	it('should return the schema children nodes on a Dictionary', () => {
-		const result = getDictionarySchemaRelations(dictionarySportStats.dictionary);
+	describe('Determine child relations by schema in a dictionary', () => {


Hurray for tests!

leoraba · 2024-10-30T13:10:34Z

PR updated. Fixed typos and replied to comments.

demariadaniel

LGTM 🚀

leoraba · 2024-11-04T15:29:53Z

README.md

+| `ID_USELOCAL`               | Generate ID locally                                                                                                                                                                                                                                                                                  | true                                   |
+| `LECTERN_URL`               | Schema Service (Lectern) URL                                                                                                                                                                                                                                                                         |                                        |
+| `LOG_LEVEL`                 | Log Level                                                                                                                                                                                                                                                                                            | 'info'                                 |
+| `PLURALIZE_SCHEMAS_ENABLED` | This feature automatically convert schema names to their plural forms when handling compound documents                                                                                                                                                                                               | true                                   |


New environment variable. When enabled it automatically pluralize schema names on compound documents

Looked up how you were doing pluralization. Nice to have a library to handle the english grammar. Might be worth commenting that the pluralization assumes the property names are in english.

leoraba · 2024-11-14T19:47:14Z

apps/server/swagger/data-api.yml

@@ -11,7 +11,7 @@
          type: string
        description: ID of the category
      - name: entityName
-        description: Array of strings to filter by entity names
+        description: Array of strings to filter by entity names. Incompatible with `compound` view


@justincorrigible: to prevent confusion about API usage, instead of ignoring, should the request fail if this parameter given along compound? an error message could say "compound views do not support filtering by entity names" or something like that

Changing entityName description. Request parameters are being validated and will give a message with the corresponding error

leoraba · 2024-11-14T19:53:56Z

packages/data-provider/src/utils/schemas.ts

+		.superRefine((data, ctx) => {
+			if (data.view === VIEW_TYPE.Values.compound && data.entityName && data.entityName?.length > 0) {
+				ctx.addIssue({
+					code: z.ZodIssueCode.custom,
+					message: 'is incompatible with `compound` view',
+					path: ['entityName'],
+				});
+			}
+		}),


Validating request fields that are incompatible with compound view

justincorrigible · 2024-11-14T22:07:11Z

apps/server/swagger/dictionary-api.yml

@@ -19,10 +19,13 @@
                type: string
                description: A matching Dictionary Name defined on Dictionary Manager (Lectern)
                required: true
-              version:
+              dictionaryVersion:


to confirm for my own understanding: this is different from a schema version, right?

it's different from schema version, changed variable name to dictionaryVersion

justincorrigible · 2024-11-14T22:07:41Z

packages/data-model/docs/README.md

+- `dictionary`: A JSONB column that stores the full dictionary schema as a JSON object. This schema defines the fields, data types, relationships, and validation rules required for data submissions, providing a flexible and easily accessible format for schema definitions.
+
+> [!NOTE]
+> Creating a new dictionary requires a [Lectern](https://github.com/overture-stack/lectern) as a Data Dictionary Management Service.


Suggested change

> Creating a new dictionary requires a [Lectern](https://github.com/overture-stack/lectern) as a Data Dictionary Management Service.

> Creating a new dictionary requires [Lectern](https://github.com/overture-stack/lectern) as a Data Dictionary Management Service.

joneubank · 2024-10-25T20:41:24Z

.env.schema

+NESTED_RECORD_PREFIX=
+NESTED_RECORD_SUFFIX=
+PARENT_RECORD_PREFIX=
+PARENT_RECORD_SUFFIX=


Need to be careful with natural language property names... not everything is pluralized with an 's'.

I'll read through how you use these properties but at first read I want to just leave the property name as the schema name, unmodified.

joneubank · 2024-10-25T20:43:24Z

apps/server/swagger/data-api.yml

@@ -31,6 +31,13 @@
        schema:
          type: integer
        description: Optional query parameter to specify the number of results per page. Default value is 20
+      - name: view
+        in: query


I'm nervous about the performance of compound object retrieval in a list... there could be many joins that are slow to resolve. I suppose if this response is paginated then this is less of a concern.

Am I correct in assuming that the purpose of the defaultCentricEntity property you are introducing is to determine which entity to use as the root when returning data in this form?

joneubank · 2024-10-25T20:44:49Z

apps/server/swagger/data-api.yml

+        schema:
+          type: string
+          enum: ['list', 'compound']
+        description: Optional query parameter to specify the data representation. Choose 'list' for an unordered list of records, or 'compound' for a nested, schema-centric structure. Default is 'list'.


This description is a bit unclear for me. Does compound view still return a list, but each entry is built into a compound document?

This feels like the options are actually "entity only" and "compound", since we get a list response in both cases.

Good point!. I see the word list is ambiguous, I could suggest even something like flat which pairs well with compound and describes the data is non-nested and

joneubank · 2024-10-25T20:45:37Z

apps/server/swagger/data-api.yml

+      - name: view
+        in: query
+        required: false
+        schema:
+          type: string
+          enum: ['list', 'compound']
+        description: Optional query parameter to specify the data representation. Choose 'list' for an unordered list of records, or 'compound' for a nested, schema-centric structure. Default is 'list'.


This is a repeat of the content comments from the previous endpoint. The same parameter is duplicated, maybe we want to write a reusable definition for this parameter.

moved view and most used parameters to a components file to be referenced

joneubank · 2024-10-25T20:51:18Z

packages/data-model/docs/schema.dbml

@@ -39,7 +39,8 @@ table dictionaries {

 table dictionary_categories {
  id serial [pk, not null, increment]
-  active_dictionary_id integer
+  active_dictionary_id integer [not null]
+  defaultCentricEntity varchar


Do we want to make this decision when we are settign the dictionary? Maybe we should instead provide the root-entity as an http request parameter, so the data requester can choose this at request time. We probably want that ability anyways, and not every dictionary will have a clear default to use.

I think setting a "default" entity only once could reduce complexity to data requesters. However, I also like the idea to override the default entity at request time.

Should this be nullable, with no default provided?

yes defaultCentricEntity can be null in database, and if provided it must match a schema name in the dictionary.

joneubank · 2024-11-14T21:14:56Z

packages/data-provider/src/services/submittedDataService.ts

+				const hierarchyStructureDesc = generateHierarchy(category.activeDictionary.dictionary, ORDER_TYPE.Values.desc);
+				const hierarchyStructureAsc = generateHierarchy(category.activeDictionary.dictionary, ORDER_TYPE.Values.asc);
+
+				const childNodes = await traverseChildNodes({
+					data: foundRecord.data,
+					entityName: foundRecord.entityName,
+					organization: foundRecord.organization,
+					schemaCentric: foundRecord.entityName,
+					treeNode: hierarchyStructureDesc,
+				});
+
+				const parentNode = await traverseParentNodes({
+					data: foundRecord.data,
+					entityName: foundRecord.entityName,
+					organization: foundRecord.organization,
+					schemaCentric: foundRecord.entityName,
+					treeNode: hierarchyStructureAsc,
+				});
+
+				dataValue = { ...dataValue, ...childNodes, ...parentNode };


Repeated code for converting an entity to compound document. Reuse a function.

joneubank · 2024-11-14T21:16:19Z

packages/data-provider/src/services/submittedDataService.ts

+	 *          If no parent nodes or dependencies exist, it returns an empty object.
+	 *
+	 */
+	const traverseParentNodes = async ({


This file is getting long with many functions that are specialized tasks used by the service, such as this one. Consider splitting the file into multiple files within a folder services/submittedData.

Splitting Service files in different files to reduce accumulating much logic in a single file.

joneubank · 2024-11-14T22:18:33Z

packages/data-provider/src/utils/dictionarySchemaRelations.ts

+ * @param order
+ * @returns
+ */
+const findOrCreateNode = (tree: TreeNode[], schemaName: string, order: OrderType): TreeNode => {


Perhaps we want to store our nodes as an object with the schemaNames as the property keys, instead of in an array where we need to traverse the array to find the node?

joneubank · 2024-11-14T22:24:11Z

packages/data-provider/src/utils/dictionarySchemaRelations.ts

+		// Use the first mapping for parent-child field relationship
+		const mapping = foreignKey.mappings[0];


This is a reasonable decision to start with, but we should consider how to manage more complex data models where an entity has multiple foreign key dependencies. Perhaps we could make an issue to track that we need to explore how this would work?

issue #111 created

joneubank · 2024-11-14T22:25:58Z

packages/data-provider/src/utils/schemas.ts

+const viewSchema = z
+	.string()
+	.trim()
+	.min(1)
+	.refine((value) => isValidView(value), 'invalid `view` parameter');


Shouldn't this just be an enum from the VIEW_TYPE?

we are converting into lowercase first and then doing the check from the enum VIEW_TYPE

you could do something like:

z.string().toLowerCase().trim().min(1).pipe(z.enum(['asdf', 'qwerty']));

justincorrigible · 2024-11-14T22:42:37Z

packages/data-model/migrations/meta/_journal.json

+      "version": "5",
+      "when": 1730128901514,
+      "tag": "0009_add_default_centric_entity",
+      "breakpoints": true
    }
  ]
 }


Suggested change

}

}

justincorrigible · 2024-11-14T23:14:10Z

packages/data-provider/src/utils/dictionarySchemaRelations.ts

+		const relatedSchema = findOrCreateNode(tree, foreignKey.schema, order);
+
+		// Use the first mapping for parent-child field relationship
+		const mapping = foreignKey.mappings[0];


is this first item guaranteed?

This logic is something we need to revisit. It works in all tested scenarios, however it's not fully guaranteed it works in more complex dictionaries. an issue was created #111

justincorrigible · 2024-11-14T23:23:50Z

packages/data-provider/src/utils/submittedDataUtils.ts

+	schemaCentricEntity: string | null;
+	recordEntityName: string;
+}): string => {
+	return filterByEntityName?.some((e) => e && e !== '') ? recordEntityName : schemaCentricEntity || recordEntityName;


if e === '' then e will be falsy, and the short-circuit won't ever get to the second bit.
it's not entirely clear what the string could be here either by its type, the variable names or the logic...
what is it that's supposed to happen here?

This function was removed as it is no longer needed due to code refactoring.

leoraba · 2024-11-19T20:25:19Z

PR Updated to fix and refactor code.

joneubank · 2024-11-19T21:58:16Z

packages/data-provider/src/services/submittedData/viewMode.ts

+				} catch (error) {
+					logger.error(`Error converting record ${record.systemId} into compound document`, error);
+				}
+				return record;


I think we should be returning some error information that the conversion failed... We don't want the client to think there was nothing to join with when that operation actually failed.

Changed to not return any records instead it throws a 500 Http error with a message An error occurred while converting records into compound view. This way user will know data or request is not valid.
Logs will describe the error details.

joneubank

This is very good now apart from the compound document failure handling in the submittedData/viewMode.ts. I'm happy to merge all this if we make a ticket to handle this failure by communicating the error to the caller instead of just returning incomplete data.

leoraba added 3 commits October 16, 2024 15:57

export controllers

12dce95

add default centric entity

dd829bc

compound view to Get data operations

7e23d19

leoraba marked this pull request as draft October 22, 2024 12:38

find parent relationships within a schema hierarchy

177f102

leoraba commented Oct 23, 2024

View reviewed changes

leoraba requested review from joneubank and justincorrigible October 23, 2024 13:07

leoraba marked this pull request as ready for review October 23, 2024 13:08

leoraba added 2 commits October 28, 2024 11:19

data model documentation

324e4fa

rename column name to snake case standard

24bf9a5

leoraba requested review from ciaranschutte and demariadaniel October 28, 2024 15:50

demariadaniel reviewed Oct 28, 2024

View reviewed changes

fix typos

946280c

demariadaniel approved these changes Oct 30, 2024

View reviewed changes

pluralize schema names

72e81e1

leoraba commented Nov 4, 2024

View reviewed changes

query param entityName incompatibility

56886b5

leoraba commented Nov 14, 2024

View reviewed changes

leoraba mentioned this pull request Nov 14, 2024

Enable support to upload .csv files #107

Merged

refactoring code

04377e4

justincorrigible reviewed Nov 14, 2024

View reviewed changes

joneubank reviewed Nov 14, 2024

View reviewed changes

justincorrigible reviewed Nov 14, 2024

View reviewed changes

leoraba added 3 commits November 18, 2024 21:03

Changing view parameter

6a894a1

code refactor - consistent use of undefined and null values

7516555

code refactor - splitting services files

3e2287f

joneubank reviewed Nov 19, 2024

View reviewed changes

joneubank approved these changes Nov 19, 2024

View reviewed changes

leoraba added 4 commits November 19, 2024 18:03

validate request parameter defaultCentricEntity

216eec9

throw error on compound view

392a836

parse view request param

1f76a4e

remove type assertions

6a0400e

leoraba force-pushed the get_compound_document branch from 7a20848 to 6a0400e Compare November 20, 2024 22:08

express parse json

88e76aa

leoraba merged commit ddaede3 into main Nov 20, 2024

leoraba deleted the get_compound_document branch November 20, 2024 22:19


		- `original_schema_id`: This field references the original schema version used during the initial submission of the data, allowing for a historical perspective on schema changes over time.

		- `submission_id`: This field links the audit record to the specific submission, creating a direct connection to the submissin responsible for this action.

		- `created_at`: This timestamp records when the audit entry was created, providing a chronological context for the logged action.

		- `created_by`: This field indicates who performed the action, enhancing accountability by identifying the user responsible for the change.

		@@ -0,0 +1,2 @@
		ALTER TABLE "dictionary_categories" ALTER COLUMN "active_dictionary_id" SET NOT NULL;--> statement-breakpoint


		expect(result).to.eql({ sports: [] });

		describe('find the hierarchycal descendant structure between schemas in the dictionary', () => {

	> Creating a new dictionary requires a [Lectern](https://github.com/overture-stack/lectern) as a Data Dictionary Management Service.
	> Creating a new dictionary requires [Lectern](https://github.com/overture-stack/lectern) as a Data Dictionary Management Service.

		// Use the first mapping for parent-child field relationship
		const mapping = foreignKey.mappings[0];

Fetch compound document #105

Fetch compound document #105

Conversation

leoraba commented Oct 22, 2024 • edited Loading

Description

Code changes:

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

demariadaniel Oct 28, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

demariadaniel Oct 28, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

leoraba commented Oct 30, 2024

demariadaniel left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joneubank Nov 19, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

justincorrigible Nov 14, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

leoraba commented Nov 19, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joneubank left a comment

Choose a reason for hiding this comment

leoraba commented Oct 22, 2024 •

edited

Loading

demariadaniel Oct 28, 2024 •

edited

Loading

demariadaniel Oct 28, 2024 •

edited

Loading

joneubank Nov 19, 2024 •

edited

Loading

justincorrigible Nov 14, 2024 •

edited

Loading