Indexing repositories #286

leoraba · 2024-11-26T14:27:33Z

Summary:

Repository package: Added new repository package as part of a monorepo package. The purpose is to create an interface to manage interactions with data source repositories, and leverage integrations to systems like Song or Lyric. New environment variables were added to facilitate repository configuration.
Indexing Lyric as source data repository: Implemented Lyric as data source repository for indexing the entire repository by category (all studies/organizations), a specific study/organization, or an individual document by it's unique ID. Issue Maestro V5 - Lyric integration #283
Documentation: Updated README files and docs/ folder according new implementations.
Build image: Updated Dockerfile to build Maestro Docker image using Node.js 22 as base
NPM publish: Readiness to publish Maestro npm package

Related issues:

Epic: Maestro V5 #279
Issues: Maestro V5 - Lyric integration #283

joneubank · 2024-11-28T19:08:57Z

For future consideration, each of the bullet points in the PR description could be its own separate change request. This would vastly simplify reviews and shorten time for revisions.

joneubank

Lots of great work. Can see that this will work as is, but I found a couple functional changes (in addition to code style things). Of importance:

chunk the bulk data requests to ES to ensure that we have a maximum request size
clean up the repository requests to paginated endpoints to prevent infinite loops

package.json

packages/common/src/utils/validation.ts

joneubank · 2024-12-12T04:49:15Z

apps/server/src/utils/logger.ts

I like the pretty printing in the dev console, but it can be a lot of noise in a log file in a production env where the colors arent being rendered. I'm curious to see how these logs look in a deployed environment and if we might want a flag to disable the pretty printing.

Might be nice to have some env log options for enabling pretty printing, logging as json, and choosing the log level, etc.

(Honestly we should standardize all this into a logger we can reuse across overture packages but thats another issue)

Development environment: Moved pretty printing to dev only (as recommended here).

Production: standard JSON format with ISO date format.

joneubank · 2024-12-12T04:57:43Z

apps/server/src/config/envConfig.ts

Some cool work parsing arrays of repositories. May want to have a separate file for the repo env processing, its kind of messy mixed in with the other env stuff.

created a new file to separate repo env processing as suggested

joneubank · 2024-12-12T04:59:18Z

apps/server/src/config/provider.ts

+const getRepositoryConfig = (
+	repos: (z.infer<typeof lyricSchemaDefinition> | z.infer<typeof songSchemaDefinition>)[],
+): (LyricRepositoryConfig | SongRepositoryConfig)[] => {
+	const lyricRepos = repos


Declare this as const lyricRepos: LyricRepositoryConfig[] and then you won't need the type assertion on the map return. This will then validate if your return type is correct as well.

fix type assertion as requested. thanks for the suggestion!

joneubank · 2024-12-12T16:29:20Z

packages/repository/src/repositories/lyric/repository.ts

+			} catch (error) {
+				logger.error(`Error fetching Lyric records on category '${categoryId}'`, error);
+			}


If the request always fails we will get stuck in an infinite look because hasMoreData is never updated.

fixed the code to throw an error to stop infinite loop

joneubank · 2024-12-12T16:30:22Z

packages/repository/src/repositories/song/repository.ts

Repeat comment re: infinite loop potential across multiple functions.

throw an error to stop infinite loop

joneubank · 2024-12-12T16:30:52Z

packages/repository/src/utils/utils.ts

+/**
+ * Finds the repository by its repository code
+ * @param repositories
+ * @param repoCode
+ * @returns
+ */
+export const getRepoInformation = (


Description should clearly state that if the repository cannot be found this will return undefined

Specified in comment when undefined is returned

joneubank · 2024-12-12T16:32:56Z

packages/repository/src/utils/utils.ts

+/**
+ * Determines if passed configuration is appropiate for Lyric
+ * @param object
+ * @returns
+ */
+export const isLyricConfiguration = (
+	object: LyricRepositoryConfig | SongRepositoryConfig,
+): object is LyricRepositoryConfig => {
+	return object.type === 'LYRIC';
+};
+
+/**
+ * Determines if passed configuration is appropiate for Song
+ * @param object
+ * @returns
+ */
+export const isSongConfiguration = (
+	object: LyricRepositoryConfig | SongRepositoryConfig,
+): object is SongRepositoryConfig => {
+	return object.type === 'SONG';
+};


Since TS 5.5 you shouldn't need to declare the type assertion for this predicate, if this type narrowing is valid then TS will infer it on its own. (ie. remove object is songRepositoryConfig.

Since this is just using differentiated union on object.type you could probably replace these functions with a switch(repository.case) in the code that uses these.

Removed these 2 functions as they are no longer needed, replaced them with a switch case as suggested,Typescript handles automatically discriminated union to know each type

joneubank · 2024-12-12T16:33:25Z

packages/repository/src/utils/utils.ts

+/**
+ * Checks if a given value is an array of objects where each object has values that are either
+ * strings, numbers, booleans, `null`, or nested objects
+ * @param value
+ * @returns
+ */
+export const isArrayOfObjects = (value: unknown): value is Array<Record<string, DataRecordValue>> => {


Whats the purpose of this?

This function is specifically used when we expect the value to be strictly an array of objects. For instance, when fetching records from Song/Lyric, we expect to retrieve an array of records, any other type is considered an invalid response.

justincorrigible · 2025-01-20T14:09:37Z

README.md

+         - kafka
+         - elastic search
+         - other helper tools if you want like kafka rest proxy


Suggested change

- kafka

- elastic search

- other helper tools if you want like kafka rest proxy

- Kafka

- Elasticsearch

- other helper tools you may want, like a Kafka RESTful proxy

apps/server/src/config/manifest.ts

apps/server/src/config/provider.ts

justincorrigible · 2025-01-20T14:27:39Z

apps/server/src/config/repositoryConfig.ts

+	const repoCount = getRepoCount();
+
+	// Loop through the repositories found
+	for (let i = 0; i < repoCount; i++) {


please replace this with repoCount.forEach to make things more declarative (and consistent with the rest of our codebases)
#ImperativeSucks

repoCount is a number, so this may be a reasonable case for a forLoop. We would want to have a utility function to re-write this in a declarative way.

you're right... and in my defence, coffee hadn't kicked in.
my argument against imperative/procedural code stands, we're meant to tell the system "what" to do, not "how"

apps/server/src/utils/requestValidation.ts

docs/features.md

docs/index.md

docs/intro.md

docs/setup.md

docs/tech.md

README.md

justincorrigible · 2025-01-20T15:00:31Z

README.md

+
+- **Maestro Common:** Designed to centralize common utilities, reusable functions, and TypeScript type definitions.
+
+- **Maestro Indexer Client:** Abstracts communication with Elasticsearch clients, supporting both version 7 and version 8.


Suggested change

- **Maestro Indexer Client:** Abstracts communication with Elasticsearch clients, supporting both version 7 and version 8.

- **Maestro Indexer Client:** Abstracts communication with Elasticsearch clients, supporting both version 7 and 8.

justincorrigible · 2025-01-20T15:01:56Z

docs/usage.md

-    - switch your Aliases if it still points to the old index
-    - now that your data is migrated Maestro will be indexing based on the new mapping.
+  - Create new index with the updated mapping
+  - Reindex your data (you can point maestro to your new index and trigger indexing on full repositories to do that,


Suggested change

- Reindex your data (you can point maestro to your new index and trigger indexing on full repositories to do that,

- Reindex your data (you can point Maestro to your new index and trigger indexing on full repositories to do that,

justincorrigible · 2025-01-20T15:02:33Z

docs/usage.md

@@ -63,66 +62,67 @@ curl -X POST \

 ## Kafka topics

-Maestro can be configured as mentioned under the running configurations section to listen to kafka topics 
+Maestro can be configured as mentioned under the running configurations section to listen to kafka topics


Suggested change

Maestro can be configured as mentioned under the running configurations section to listen to kafka topics

Maestro can be configured as mentioned under the running configurations section to listen to Kafka topics

justincorrigible · 2025-01-20T15:33:20Z

packages/repository/src/utils/utils.ts

+ * @returns
+ */
+export const isArrayOfObjects = (value: unknown): value is Array<DataRecordNested> => {
+	return (


curious to see this implemented in vanilla code, instead of Zod (and I like that).
why the choice though?

tbh didn't thought about using Zod for this, initially this function was checking only arrays types were added later.

packages/repository/src/repositories/song/repository.ts

joneubank

Generally speaking this has improved a bit since I first read it, though I am concerned we haven't addressed pagination, or ES bulk request chunking. I see Result objects for some responses which is good.

I know you plan on chunking some of those changes into a separate PR adn thats fine. Since this is a major refactor and being worked on in a branch separate form the primary, I say go ahead and merge this when you feel its ready, and I'll continue to review future change requests.

joneubank · 2025-01-20T20:25:42Z

apps/server/src/config/envConfig.ts

 	MAESTRO_ELASTICSEARCH_VERSION: z.coerce
 		.number()
+		.int()
 		.refine((val) => val === 7 || val === 8, { message: 'Version must be 7 or 8' }), // Ensure 7 or 8


Couldn't this just be a zod enum? Doing a refine to a limited set of values seems way trickier. We also would probably like to have a list of valid versions to use as a switch in other places.

created a constant in common package to be used anywhere in the monorepo

joneubank · 2025-01-20T20:26:09Z

apps/server/src/config/envConfig.ts

 const loggerConfigSchema = z.object({
-	MAESTRO_LOGGING_LEVEL_ROOT: z.string().default('info'),
+	MAESTRO_LOGGING_LEVEL: z.string().default('info'),


If there are a set of valid logging levels, lets define those. zod enum use case.

Added enum with valid logger levels.

joneubank · 2025-01-20T20:28:09Z

apps/server/src/config/provider.ts

 import { env } from './envConfig.js';
+import { type lyricSchemaDefinition, repositoryTypes, type songSchemaDefinition } from './repositoryConfig.js';
+
+setLogLevel(env.MAESTRO_LOGGING_LEVEL);


This is happening at a global import level, not inside the provider instance. Isn't the logger to use part of the dependencies passed to each provider instance? The log level should be set there based on the dependencies/config.

moved this setter function into the MaestroProviderConfig object. Good catch!

apps/server/src/config/provider.ts

joneubank · 2025-01-20T20:50:40Z

apps/server/src/config/repositoryConfig.ts

+ * @returns an array of valid repositories
+ * @throws an error if repository is invalid
+ */
+export const validateRepositories = (env: NodeJS.ProcessEnv) => {


NodeJS.ProcessEnv I didn't know of this type, interesting.

it's part of the typescript global types 😉

joneubank · 2025-01-20T21:02:33Z

packages/common/src/types/config.ts


-type SongRepositoryConfig = {
+type ValueOf<T> = T[keyof T];


this is a common utility type that you could export from its own file for global use. Typically I call this Values<T>.

joneubank · 2025-01-20T21:04:18Z

packages/common/src/utils/validation.ts

You could just call this file isEmpty.ts

if that is the case, this export in line 6 should become default.

however, there is a case for this validation to be a barrelled folder instead

utils |-validation |-index (exports centrally from all files in this folder) |-isEmpty

created a folder validation to hold isEmpty.ts

joneubank · 2025-01-20T21:07:40Z

packages/common/src/utils/validation.ts

+ * @param value The value to check. This can be of any type.
+ * @returns `true` if the value is `null`, `undefined`, an empty object, an empty array or an empty string; otherwise, `false`.
+ */
+export const isEmpty = (value: unknown) => {


Looks ok, is there a test for this?

[null, undefined, '', ' ', '0', true, false, {}, []].map((value)=>({value,isEmpty:isEmpty(value)}))

[ { value: null, isEmpty: true }, { value: undefined, isEmpty: true }, { value: "", isEmpty: true }, { value: " ", isEmpty: false }, { value: "0", isEmpty: false }, { value: true, isEmpty: false }, { value: false, isEmpty: false }, { value: {}, isEmpty: true }, { value: [], isEmpty: true } ]

yes, this has a suite of unit tests

packages/indexer-client/src/client/v7/operations.ts

joneubank · 2025-01-20T21:10:53Z

packages/indexer-client/src/client/v7/operations.ts

+
+		return {
+			indexName: index,
+			successful,


Did this get addressed?

justincorrigible · 2025-01-21T13:40:30Z

packages/common/src/types/logger.ts

inclined to suggest this could follow the standard "module" pattern, i.e. having this file named "types.ts" and placed within the "src/logger" folder. that pattern facilitates finding all code in their relative/contextual scopes. in this current pattern you have "buckets full of X", which may or not include the ones for this or that component or controller, or provider, or whatever.

let me explain myself: following what you did in logger, if i want to create a new button for example, and knowing me you know my button needs to look special, right? so it has all sorts of fancy styles and contains an emoji image in it to make it "cool!", so then I go create the "src/Button" folder and within I create the ts file for it (Button/Button.ts), then I'd go back out to src create an "images" folder where I will put all the images including my emoji for the button, then put the css file in a "src/styles" folder, and then put the types in this "src/types" folder here where all the types are?
later on, if we realize my button was silly and want to remove it from the codebase, I'll be chasing down files all over the repo instead of just deleting the one single folder and calling it a day. I don't know, this file/folder structure pattern seems scattered and hard to maintain, but I'm willing to be convinced otherwise.

@joneubank probably has a different opinion from mine?

justincorrigible · 2025-01-21T13:54:28Z

packages/repository/README.md

+> [!NOTE]
+> This package is likely not the one you want to use in your project, it is primarily used as a dependency within the Maestro monorepo. [@overture-stack/maestro-provider](https://www.npmjs.com/package/@overture-stack/maestro-provider) is what you are most likely to want to use.
+
+This package is an internal component of the Maestro monorepo designed to manage interactions with data source repositories. It serves as the central interface for retrieving data from various repositories, such as **Song** and **Lyric**, ensuring a streamlined and consistent approach to data access.


Suggested change

This package is an internal component of the Maestro monorepo designed to manage interactions with data source repositories. It serves as the central interface for retrieving data from various repositories, such as **Song** and **Lyric**, ensuring a streamlined and consistent approach to data access.

This package is an internal component of the Maestro monorepo designed to manage interactions with data source repositories. It serves as the central interface for retrieving data from various repositories, such as **SONG** and **Lyric**, ensuring a streamlined and consistent approach to data access.

justincorrigible · 2025-01-21T13:57:24Z

packages/repository/package.json

+	"version": "5.0.0-alpha.1",
+	"description": "A submodule of Maestro that manages interactions with data source repositories, providing a unified interface for efficient data access and management",
+	"main": "dist/index.js",
+	"typings": "dist/index.d.ts",


thought this was supposed to be "types"? curious to understand what the difference is

it can be both. ref: Note that the "typings" field is synonymous with "types", and could be used as well.

leoraba · 2025-01-21T21:17:03Z

Closing this PR as most of the feeback was resolved, the rest of work will be done on a refactoring PR to reduce complexity.

leoraba added 6 commits November 6, 2024 07:16

bulk upsert

208be87

add repository project

a07023f

initialize logger

bb77dea

update readme and docs

dcf4363

invalid repository configuration

672cca5

docker file

a1bee20

leoraba requested review from joneubank and justincorrigible November 26, 2024 14:29

leoraba added 3 commits November 26, 2024 11:36

enable jenkins pipeline

0a58da8

private and public packages

d4ae334

udpate documentation

d7ba0ba

joneubank requested changes Dec 12, 2024

View reviewed changes

leoraba added 11 commits January 2, 2025 09:30

sanitize key names

cfe1345

scripts definition

814f4c7

fix client integration test

d20b746

common validation unit tests

74aed1c

customize logs formatters

8a2fe16

separate repository variables processing

be0f0be

fix type assertion

5cca842

refactoring discriminated union

d44cdfc

request validation using safeParse

b72fb95

logger refactor

14843d5

rename interfaces

935f5ca

leoraba force-pushed the indexing_repositories branch from 738b19a to 935f5ca Compare January 7, 2025 04:57

leoraba added 5 commits January 7, 2025 01:58

refactor code and fix infinite loop issue

858085d

nested data type

609e522

do not alter index name

ebdfd83

add repository unit tests

2df1023

adding more indexer unit tests

9cba329