Skip to content

improv/execQuery legibility #1105

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

Draft
wants to merge 2 commits into
base: 21.x
Choose a base branch
from
Draft

Conversation

will-apresenta
Copy link

πŸ”— Linked issue

❓ Type of change

  • 🐞 Bug fix (a non-breaking change that fixes an issue)
  • πŸ‘Œ Enhancement (improving an existing functionality like performance)
  • ✨ New feature (a non-breaking change that adds functionality)
  • ⚠️ Breaking change (fix or feature that would cause existing functionality to change)

πŸ“š Description

Improves legibility to execQuery and exports QueryRunner to make it easier to extend
Resolves #1102

πŸ“ Checklist

  • I have linked an issue or discussion.
  • I have updated the documentation accordingly.

@RomainLanz
Copy link
Member

Hey @will-apresenta! πŸ‘‹πŸ»

May you add some tests to your PR?

@will-apresenta
Copy link
Author

Hi @RomainLanz, I've didn't created any test because it don't change the functionality itself, but i can create some if needed

@will-apresenta
Copy link
Author

Hi, @RomainLanz, i didn't found any test to execQuery, but all the existing tests run well

@RomainLanz
Copy link
Member

I've didn't created any test because it don't change the functionality itself, but i can create some if needed

Correct, but it will avoid us to break those contract. At least, you could add a test that mimic your usecase (use a custom query builder).

@will-apresenta will-apresenta marked this pull request as draft May 12, 2025 14:24
@will-apresenta
Copy link
Author

will-apresenta commented May 12, 2025

Hi @RomainLanz, I'm struggling how to always call execQuery.

In short, i'm trying to create a cache on model level, it means, i will be able to configure cache on each model, including disable it, so i will need model class to be accessible where it created query and executes. Before making the PR, i've tested it on my adonisjs application and overriding execQuery with macro worked, but i've forgoten to test saving relations, so now creating tests to lucid i've realized that row.related('relation').create({...}) don't call's execQuery

My test:

import { test } from '@japa/runner'
import { AppFactory } from '@adonisjs/core/factories/app'

import { column, hasMany } from '../../src/orm/decorators/index.js'
import { ModelQueryBuilder } from '../../src/orm/query_builder/index.js'
import {
  getDb,
  setup,
  cleanup,
  ormAdapter,
  resetTables,
  getBaseModel,
} from '../../test-helpers/index.js'
import type { HasMany } from '../../src/types/relations.js'
import { base64 } from '@poppinss/utils'
import { QueryRunner } from '../../src/query_runner/index.js'
import { FileSystem } from '@japa/file-system'
import { LucidRow } from '../../src/types/model.js'

const setupCache = async (fs: FileSystem) => {
  const fakeCache = new Map<string, LucidRow | LucidRow[]>()

  const app = new AppFactory().create(fs.baseUrl, () => {})
  await app.init()
  const db = getDb()
  const adapter = ormAdapter(db)
  const BaseModel = getBaseModel(adapter)

  class Post extends BaseModel {
    @column()
    declare userId: number | null

    @column()
    declare title: string
  }

  class User extends BaseModel {
    @column({ isPrimary: true })
    declare id: number

    @column()
    declare username: string

    @hasMany(() => Post)
    declare posts: HasMany<typeof Post>
  }

  Post.boot()
  User.boot()

  ModelQueryBuilder.macro(
    'execQuery' as keyof ModelQueryBuilder,
    async function (this: ModelQueryBuilder) {
      this.applyWhere()
      const isWriteQuery = this.isWriteQuery()

      const cacheTags = [this.model.table]
      cacheTags.push(base64.urlEncode(this.toQuery()))

      if (!isWriteQuery) {
        const cacheKey = cacheTags.join(':')
        let cachedData = fakeCache.get(cacheKey)

        if (cachedData) {
          let data = cachedData

          if (!Array.isArray(data)) data = [data]

          if (this.wrapResultsToModelInstances) {
            return this.convertRowsToModelInstances(data)
          } else {
            return data
          }
        }
      }

      const queryData = Object.assign(this.getQueryData(), this.customReporterData)
      const rows = await new QueryRunner(this.client, this.debugQueries, queryData).run(
        this.knexQuery
      )

      if (isWriteQuery || !this.wrapResultsToModelInstances) {
        if (isWriteQuery) {
          cacheTags.pop()
          const cacheKey = cacheTags.join(':')
          fakeCache.forEach((_, key) => {
            if (key.startsWith(cacheKey)) {
              fakeCache.delete(key)
            }
          })
        }
        return Array.isArray(rows) ? rows : [rows]
      }

      const modelInstances = this.convertRowsToModelInstances(rows)

      if (!isWriteQuery) {
        const cacheKey = cacheTags.join(':')
        fakeCache.set(cacheKey, modelInstances)
      }

      await this.preloadFromModels(modelInstances)

      return modelInstances
    }
  )

  return {
    fakeCache,
    Post,
    User,
  }
}

test.group('Model query builder execQuery', (group) => {
  group.setup(async () => {
    await setup()
  })

  group.teardown(async () => {
    await cleanup()
  })

  group.each.teardown(async () => {
    await resetTables()
  })

  group.each.disableTimeout()

  test('apply relationship constraints when using sub query', async ({ fs, assert }) => {
    const { fakeCache, Post, User } = await setupCache(fs)

    const users = await User.createMany([
      {
        username: 'virk',
      },
      {
        username: 'nikk',
      },
    ])

    assert.lengthOf(fakeCache, 0)

    for (let user of users) {
      await user.related('posts').create({ title: 'Test' })
    }

    const postsQuery = Post.query().whereIn('id', users[0].related('posts').query().select('id'))

    const posts = await postsQuery

    assert.lengthOf(fakeCache, 1)

    const newPosts = await postsQuery

    assert.lengthOf(fakeCache, 1)
    assert.deepEqual(posts, newPosts)

    await users[0].related('posts').create({ title: 'Test 2' })

    // It breaks here, because related().create doesn't call execQuery, so it don't clears the cache
    assert.lengthOf(fakeCache, 0)
  })
})

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.

Export QueryRunner to extend ModelQueryBuilder
3 participants