vapor / fluent-kit

Swift ORM (queries, models, and relations) for NoSQL and SQL databases
MIT License
218 stars 116 forks source link

Fix NULL handling for OptionalFieldProperty and OptionalEnumProperty #473

Closed gwynne closed 2 years ago

gwynne commented 2 years ago

OptionalFieldProperty.value.getter was incorrectly returning .none(.none) instead of .some(.none) when self.inputValue was DatabaseQuery.Value.null, preventing explicitly set nil values set by properties that wrap optional field properties (i.e. OptionalEnumProperty) from taking effect as they should.

Additionally, OptionalEnumProperty.value.setter was not correctly forwarding the double-optional state of newValue, causing a fatalError() due to non-uniform input sets on bulk inserts.

This fixes both of those issues and updates the appropriate tests. The behavior of OptionalEnumProperty is noticeably different now, so even though this does not change any public API, it will be released as semver-minor to lessen impact for those who were relying on the incorrect old behavior.

Fixes #396. Fixes #444.

codecov-commenter commented 2 years ago

Codecov Report

Merging #473 (45d1a4b) into main (a3630ca) will increase coverage by 6.78%. The diff coverage is 73.80%.

@@            Coverage Diff             @@
##             main     #473      +/-   ##
==========================================
+ Coverage   33.17%   39.95%   +6.78%     
==========================================
  Files         104       95       -9     
  Lines        5547     5168     -379     
==========================================
+ Hits         1840     2065     +225     
+ Misses       3707     3103     -604     
Flag Coverage Δ
unittests 39.95% <73.80%> (+6.78%) :arrow_up:

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
...s/FluentKit/Concurrency/AsyncModelMiddleware.swift 0.00% <ø> (ø)
Sources/FluentKit/Query/Builder/QueryBuilder.swift 70.16% <ø> (+3.36%) :arrow_up:
Sources/XCTFluent/DummyDatabase.swift 18.62% <ø> (ø)
Sources/XCTFluent/TestDatabase.swift 63.04% <ø> (+7.24%) :arrow_up:
Sources/FluentKit/Properties/Timestamp.swift 50.00% <50.00%> (+8.41%) :arrow_up:
Sources/FluentKit/Enum/OptionalEnumProperty.swift 74.13% <85.71%> (+2.43%) :arrow_up:
Sources/FluentKit/Properties/OptionalField.swift 89.02% <100.00%> (+6.30%) :arrow_up:
...es/FluentKit/Query/Builder/QueryBuilder+Join.swift 84.61% <100.00%> (+9.98%) :arrow_up:
Sources/FluentBenchmark/FluentBenchmarker.swift
Sources/FluentBenchmark/SolarSystem/Planet.swift
... and 36 more
jaredh159 commented 2 years ago

@gwynne spent a little time trying to test this fix, as requested, but am struggling as to how to pull in the new code. I tried depending on a branch and a commit in my Package.swift (even though my project doesn't explicitly depend on fluent-kit, only on fluent), but I kept running into errors.

Is there a simpler way to test out a patch like this in a local vapor app?

0xTim commented 2 years ago

You can checkout FluentKit and run it in a workspace as described here https://www.timc.dev/posts/editing-swift-packages/

Alternatively depending on the branch should work - how are you defining it? (Option 1 is much easier)

jaredh159 commented 2 years ago

@0xTim struggling to follow the instructions in your post. Admittedly, Xcode and me are not the best of friends, I try to do almost all of my Swift development in vscode. The part of your instructions where you say "find the directory for the package you want to work on and drag that directory into the project navigator in Xcode" -- I'm a bit lost on. I created an empty workspace. Do I then open a finder window and drag the root folder of my vapor App into the left-hand side of Xcode? I tried that, but it didn't look like your screenshot, and Xcode didn't seem to notice the Package.swift file or do any fetching of dependencies or anything like that. Am I misunderstanding your instructions? Sorry for being such an Xcode noob.

0xTim commented 2 years ago

@jaredh159 yeah that's correct, in the workspace just drop the directory containing the package.swift into the bar and then drop the dependency you want to edit. The screenshots may be out of date with Xcode 12

gwynne commented 2 years ago

I'm going to go ahead and merge these changes, hopefully they do solve the issue in question. If not, ping me on Discord and I'll have another look.

VaporBot commented 2 years ago

These changes are now available in 1.21.0