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

TypeORM discussion #1131

Closed
pbn4 opened this issue Apr 15, 2021 · 4 comments
Closed

TypeORM discussion #1131

pbn4 opened this issue Apr 15, 2021 · 4 comments
Assignees

Comments

@pbn4
Copy link
Contributor

pbn4 commented Apr 15, 2021

The problem with our current setup (my subjective opinion):

  1. Excessive memory usage when querying listings and applications because of nested joins, see issues:

    The problem is two-fold: TypeORM might be better with memory usage when handling nested relations and our data model requires many JOIN operations to fetch all data requires for server response.

  2. Querybuilder ease of use, right now to join relations we have to write something like this:

     const qb = this.repository.createQueryBuilder("application")
     qb.leftJoinAndSelect("application.user", "user")
     qb.leftJoinAndSelect("application.listing", "listing")
     qb.leftJoinAndSelect("application.applicant", "applicant")
     qb.leftJoinAndSelect("applicant.address", "applicant_address")
    

    which I find problematic because this is not typed in any way and you have no guarantee that what's actually returned matches your runtime model, you have to check it yourself.

Potential solutions

Point 1 is crucial, point 2 is something we can live with eventually. I've been watching those linked issues for quite a bit now and there seems to be no progress in improving performance, neither do I feel competent to fix it myself and submit a PR to TypeORM repo, so we have two options:

  1. Optimize the model (less joins)
  2. Find an ORM which performs better.

Option 1. Query optimizations for Application and Listing models.

Applications

Currently we have a following query built on each application retrieval:

```
// --> Build main query
const qb = this.repository.createQueryBuilder("application")
qb.leftJoinAndSelect("application.user", "user")
qb.leftJoinAndSelect("application.listing", "listing")
qb.leftJoinAndSelect("application.applicant", "applicant")
qb.leftJoinAndSelect("applicant.address", "applicant_address")
qb.leftJoinAndSelect("applicant.workAddress", "applicant_workAddress")
qb.leftJoinAndSelect("application.alternateAddress", "alternateAddress")
qb.leftJoinAndSelect("application.mailingAddress", "mailingAddress")
qb.leftJoinAndSelect("application.alternateContact", "alternateContact")
qb.leftJoinAndSelect("alternateContact.mailingAddress", "alternateContact_mailingAddress")
qb.leftJoinAndSelect("application.accessibility", "accessibility")
qb.leftJoinAndSelect("application.demographics", "demographics")
qb.leftJoinAndSelect("application.householdMembers", "householdMembers")
qb.leftJoinAndSelect("householdMembers.address", "householdMembers_address")
qb.leftJoinAndSelect("householdMembers.workAddress", "householdMembers_workAddress")
```

I think we could optimize it slightly by removing:

  • removing user join because we do not return user information in application DTOs at all
  • listing join because we only return listingId from the API

Listings

We currently cannot optimize any joins:

 return Listing.createQueryBuilder("listings")
   .leftJoinAndSelect("listings.leasingAgents", "leasingAgents")
   .leftJoinAndSelect("listings.preferences", "preferences")
   .leftJoinAndSelect("listings.property", "property")
   .leftJoinAndSelect("property.buildingAddress", "buildingAddress")
   .leftJoinAndSelect("property.units", "units")
   .leftJoinAndSelect("units.amiChart", "amiChart"

so we solved long query time with http caching, which we will not be able to use when we introduce CMS (data changes should be reflected immediately in e.g. preview).

Option 2. Find TypeORM replacement:

This approach requires:

  1. Finding an ORM which:

    • has better performance in our use case scenario
    • automatically generates DB migrations
    • is typescript based
    • is actively maintained
    • supports transactions
  2. Replacing TypeORM with this new ORM:

    • writing model definition in this new ORM convention in a way that is compatible with our current DB schema
    • replacing TypeORM repositories injected in services with new db access classes

ORMs that I'm currently researching:

  • Prisma (supports transactions since september 2020, which lack of was a no-go for us when we were starting)
  • Sequelize 6
@pbn4
Copy link
Contributor Author

pbn4 commented Apr 15, 2021

#1070

@pbn4
Copy link
Contributor Author

pbn4 commented Apr 15, 2021

Public app optimizations:

  • replace jsonpath with a proper query param
  • use pagination when querying /listings

Backend:

  • make address a json column on appropriate tables joined by Application

@pbn4
Copy link
Contributor Author

pbn4 commented May 20, 2021

@seanmalbert Together with @bpsushi we are having problems with exposing listing in application DTO as IdDto without joining it earlier using a query builder.

Expected model:

{
   ...
   "listing": {
     "id": "..."
   } (IdDto)
}

Current state is that we would like to use @RelationId decorator to expose foreign key column to the ORM model (without joining the actual table) and build an IdDto out of that, but DTO definition is ignoring our @Expose decorators for getter making it impossible to build listing IdDto out of listingId (relation key).

My suspicion is that since we are extending application.entity.ts entity class (with OmitType) we are inheriting decorators for keys that are omitted too and later those are passed to class-transformer (unexpectedly). What we want to try next is geting rid of OmitType extend/inheritance type of defining a DTO for an entity class and just put standard implements class there + redefine all properties in it. This way we will still retain strict type checking + it will be clear what decorators and what metadata is exactly part of our DTO definition. Hopefully that will also make exposing getters work. :)

@kathyccheng
Copy link
Collaborator

Is there more we want to do on this or can I close? @pbn4

@pbn4 pbn4 closed this as completed Jun 10, 2021
YazeedLoonat pushed a commit to YazeedLoonat/bloom that referenced this issue May 31, 2022
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

No branches or pull requests

3 participants