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

The unique rule from VineJS should be simpler? #995

Open
thetutlage opened this issue Feb 2, 2024 · 7 comments
Open

The unique rule from VineJS should be simpler? #995

thetutlage opened this issue Feb 2, 2024 · 7 comments
Assignees
Labels
Type: Enhancement Improving an existing feature

Comments

@thetutlage
Copy link
Member

Discussed in adonisjs/core#4356

Originally posted by brunolipe-a January 26, 2024
I think that always use a query to check if the value is unique in database is to complex for something that we use very often.

Maybe we can have an uniqueRaw that uses the current code, and other that receives only table as required, something like that:

/**
 * Options accepted by the unique rule
 */
type Options = {
  table: string
  column?: string
  filter?: (db: DatabaseQueryBuilderContract, value: string, field: FieldContext) => Promise<void> 
}
@thetutlage thetutlage transferred this issue from adonisjs/core Feb 2, 2024
@thetutlage thetutlage self-assigned this Feb 2, 2024
@thetutlage thetutlage added the Type: Enhancement Improving an existing feature label Feb 2, 2024
@brunolipe-a
Copy link

I made an implementation on a side project, but I don't know if it follows the VineJS best way of doing it:

import vine from '@vinejs/vine'
import { FieldContext } from '@vinejs/vine/types'

import db from '@adonisjs/lucid/services/db'
import { DatabaseQueryBuilderContract } from '@adonisjs/lucid/types/querybuilder'

/**
 * Options accepted by the uniqueSimple rule
 */
export type UniqueSimpleOptions = {
  table: string
  column?: string
  filter?: (db: DatabaseQueryBuilderContract, value: string, field: FieldContext) => Promise<void>
}

/**
 * Implementation
 */
async function uniqueSimple(value: unknown, options: UniqueSimpleOptions, field: FieldContext) {
  /**
   * We do not want to deal with non-string
   * values. The "string" rule will handle the
   * the validation.
   */
  if (typeof value !== 'string') {
    return
  }

  if (typeof field.name !== 'string') {
    return
  }

  const columnName = options.column || field.name

  const baseQuery = db.from(options.table).select(columnName).where(columnName, value)

  await options.filter?.(baseQuery, value, field)

  const row = await baseQuery.first()

  if (row) {
    field.report('The {{ field }} has already been taken', 'database.unique', field)
  }
}

/**
 * Converting a function to a VineJS rule
 */
export const uniqueSimpleRule = vine.createRule(uniqueSimple)

@Xstoudi
Copy link

Xstoudi commented Feb 2, 2024

Happy the discussion exists as I was on my way to open an identical one.

To contribute a bit: v5 unique rule felt really straightforward and I think having it back will be a great QoL improvement: table name, field name and that's it.

@thetutlage thetutlage transferred this issue from adonisjs/auth Feb 5, 2024
@Tahul
Copy link
Contributor

Tahul commented Feb 5, 2024

@thetutlage ; I found this implemention in adocasts repository:

https://github.com/adocasts/adocasts/blob/next/app/validators/helpers/db.ts

Used that way:

import { exists } from '#app/validators/helpers/db'

vine.string().exists(exists('table', 'field', { caseInsensitive: boolean }))

That finds my needs for the migration from @adonisjs/validation to @vinejs/vine.

I guess you could tweak it to be the exact same as the previous rule (object parameters).

@EwertonDutra
Copy link

We really need simple validations, like in version 5
Please

@thetutlage
Copy link
Member Author

Can we have some PR for the same? Also, will be nice if you can also take care of the docs for the same

@Xstoudi
Copy link

Xstoudi commented Mar 11, 2024

Can do the PR tonight but don't want to lick the cookie so feel free to PR first. Just to clear things up, @thetutlage do you want it to fully replace the current implem or to live aside it?

@thetutlage
Copy link
Member Author

Live alongside the current API

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Type: Enhancement Improving an existing feature
Projects
None yet
Development

No branches or pull requests

5 participants