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

Add extra set method for DatabaseQuery.Value #468

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

madsodgaard
Copy link
Contributor

This allows the use DatabaseQuery.Value when setting a @Field property value. For example you can now use SQL:

query.set(\.$field, to: .sql(raw: "1"))

@0xTim 0xTim added the semver-minor Contains new APIs label Nov 17, 2021
Copy link
Member

@0xTim 0xTim left a comment

Choose a reason for hiding this comment

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

LGTM but I'll let @gwynne have the final say

@gwynne
Copy link
Member

gwynne commented Nov 22, 2021

@madsodgaard Adding this overload would make it much easier to violate correctness of field data types, so I'm a little hesitant. Do you have a concrete use case in mind?

@gwynne gwynne self-assigned this Nov 22, 2021
@gwynne gwynne added enhancement New feature or request question Further information is requested labels Nov 22, 2021
@gwynne gwynne added this to Awaiting Review in Vapor 4 via automation Nov 22, 2021
@madsodgaard
Copy link
Contributor Author

madsodgaard commented Nov 22, 2021

@gwynne I am using it to have SQL as a value instead of having to drop down to SQLKit. Would you prefer having explicit methods for SQL instead? I mean the obvious thing for the user would still be using the correct field data type, and requires them to explicitly state they want to bypass the data type correctness by typing .

Model
  .query(on: database)
  .set(\.$balance, to: .sql(embed: "\(Model.path(for: \.$balance).first!) - \(bind: amount)"))
  .update()

Edit: I know I can use .set() with FieldKeys and DatabaseQuery.Value, I just think the API reads better with Field and allows the user to chain multiple .set() calls instead of having one method to set all input values

@gwynne
Copy link
Member

gwynne commented Nov 22, 2021

@madsodgaard Ideally, I'd prefer to add functionality to FluentKit to express what you're currently using SQL for, rather than encouraging breaking the abstraction. In practice, of course there's only so much that can be supported, and adding an expressive feature to FluentKit isn't usually very easy. (Though, if what you want to do does boil down to "using a binary expression to express an updated column value, as in your example, I do have a pretty clear idea of how to make that work in Fluent with Swift operators - it'd probably end up being as simple as being able to write .set(\.$balance, to: \.$balance - amount) directly, similarly to what you can do in .filter() and .join() clauses.)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request question Further information is requested semver-minor Contains new APIs
Projects
Vapor 4
  
Awaiting Review
Development

Successfully merging this pull request may close these issues.

None yet

3 participants