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

Refactor: Schema Definition in TypeScript with Zod #181

Merged
merged 33 commits into from
Jul 5, 2023

Conversation

joneubank
Copy link
Contributor

@joneubank joneubank commented Jun 24, 2023

Purpose of this Refactor: Improve Types

This work started as an attempt to improve the type system usage throughout the repository, which revealed an opportunity to improve the method used to define the Dictionary Meta-Schema.

The original planned type system improvements were to:

  1. Set .tsconfig.json to use strict: true (and fix the errors this causes).
  2. Define in TS the Dictionary and Schema types.
  3. Remove all uses of any types where the internal types were known.
  4. Remove all unsafe or incorrect type casts throughout.

Working on item 2 - writing TS types for the Dictionary Meta-Schema - revealed a couple things about the previous setup:

  1. Keeping internal types consistent with changes to the Meta-Schema JSON would be an ongoing maintenance challenge.
  2. There were validations being done in code that were not reflected by the Meta-Schema.
  3. The validations were not being done in the same place, they were scattered through different steps in the code base.
  4. There were validations that were needed that were not being done.
  5. There were validations that were needed that could not be reflected in a JSON-Schema defined Meta-Schema.

Along with this information it is important to highlight that there are tools available to TypeScript in 2023 that were not available when Lectern was first written. Specifically, we now have zod, a TypeScript first schema definition library that is becoming an industry standard. Zod can resolve all of the issues highlighted above, including the two most important changes (in my opinion):

  1. Meta-Schema and TS Type information will always be in sync
  2. All schema validations can be written in a single logical place

This is important for code maintenance (no inconsistency between JSON schema and TS). It lets developers take advantage of the TS type system in their coding, for the benefits in reliability and code feedback that it provides. This will also standardize how Dictionary validation is performed, putting the content rules adjacent to the structure definition and ensuring both are run in the same way.

Changes in This PR

There are a lot of files touched in this PR but the important changes are isolated to a few. The long list of files changed is the result of related changes to imports, small type fixes, and some moving of files around.

The important changes are:

  1. /src/types/dictionaryTypes.ts - Zod Schema definitions, and exports of types for Dictionary, Schema, fields, restrictions, and all of the composite types.
  2. Removal of MetaSchema.json - Replaced by dictionaryTypes.ts.
  3. Moving DB Model code (mongoose schema) into the /src/db/dictionaryModel.ts file.
  4. Moving express route handlers to dedicated router files in /src/routers.
  5. Moving client API code into the directory /src/external - This includes ego and vault integration code.
  6. Adding request validation to /src/controllers/dictionaryController.ts.

Some other updates performed:

  1. Tests were updated to used new type system, some test fixtures were invalid and needed updating.
  2. Directory added to share sample Dictionaries and Schemas, two basic examples have been provided to start with.
  3. package.json has been updated to be version 2+, using the pre-release version tag next.
  4. Copyright notice year was updated to 2023 for consistency.
  5. Organized imports in all /src files

Bugs Fixed

@joneubank joneubank marked this pull request as ready for review June 24, 2023 22:55
@joneubank
Copy link
Contributor Author

Not mentioned in this PR description - We can generate a JSON Schema formatted Meta-Schema for the Lectern Dictionary structure so that a language agnostic schema is available to validate against. We should add a piece of automation to the build step that generates this Meta-Schema file from the Zod Schemas.

It would also be valuable to add a version of the create dictionary, add schema, and update schema methods that work as a validator without updating the stored dictionaries. These endpoints would output the dictionary as it would look after updates were made, but nothing would be committed to the DB. It would be best to put these on a URI path separate from the endpoints that update the DB.

Copy link
Member

@justincorrigible justincorrigible left a comment

Choose a reason for hiding this comment

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

heaps of trolling and nitpicking. don't take anything too seriously...

but maybe, the primitive spelling mistakes... those, do correct ;)

Comment on lines +7 to +8
"build": "npm run build-ts",
"serve": "node dist/server.js",
Copy link
Member

Choose a reason for hiding this comment

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

these steps can be avoided altogether using ts-node.
it's as close as it gets right now to treating ts like an actual language 😆

package.json Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
samples/dictionary/simple.json Show resolved Hide resolved
samples/schemas/primatives.json Show resolved Hide resolved
test/fixtures/dictionaries/simple.ts Show resolved Hide resolved
* ANY WAY OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
*/

import { Dictionary } from '../../../../src/types/dictionaryTypes';
Copy link
Member

Choose a reason for hiding this comment

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

....lol.....

src/types/dictionaryTypes.ts Outdated Show resolved Hide resolved
src/types/dictionaryTypes.ts Show resolved Hide resolved
visited: VisitedSet,
): string | string[] => {
const cachedValue = discovered.get(tag);
if (cachedValue !== undefined) {
Copy link
Member

Choose a reason for hiding this comment

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

le !isUndefined is le blergh

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For future reference: The intention here is to permit empty strings through, and certainly TS tells us it will only be a string, string array, or undefined... but assuming some strange value gets through this will also permit NaN and null through.

The absolute best check would be to check if it is truthy or an empty string: if (cachedValue || cachedValue === '')

joneubank and others added 2 commits July 4, 2023 20:17
Co-authored-by: Anders Richardsson <2107110+justincorrigible@users.noreply.github.com>
Co-authored-by: Anders Richardsson <2107110+justincorrigible@users.noreply.github.com>
@joneubank joneubank merged commit 25816d2 into develop Jul 5, 2023
@joneubank joneubank deleted the refactor/schema-types branch July 5, 2023 00:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants