Skip to content

Commit a050d33

Browse files
gwynnegwynne
authored
Address issues in decoding data from the database (vapor#211)
* Retool PostgresDataDecoder not to violate Coding usage invariants, to throw more consistent errors, and to forward its decoding logic consistently. * Fix tests to clean up connections properly on error, to respect environment overrides for test config, to handle errors sensibly, and to not open lots of pointless extra connections. * Heavily updates the CI, including fixing some brokenness caused by a GH Actions bug.
1 parent 6f35f30 commit a050d33

File tree

4 files changed

+233
-229
lines changed

4 files changed

+233
-229
lines changed

‎.github/workflows/test.yml

Lines changed: 108 additions & 55 deletions
Original file line numberDiff line numberDiff line change
@@ -1,63 +1,116 @@
11
name: test
22
on:
3-
- pull_request
3+
pull_request:
4+
push: { branches: [ main ] }
5+
46
jobs:
5-
postgres-kit_xenial:
6-
container:
7-
image: vapor/swift:5.2-xenial
8-
services:
9-
psql:
10-
image: postgres
11-
ports:
12-
- 5432:5432
13-
env:
14-
POSTGRES_USER: vapor_username
15-
POSTGRES_DB: vapor_database
16-
POSTGRES_PASSWORD: vapor_password
7+
linux-plus-dependents:
8+
strategy:
9+
fail-fast: false
10+
matrix:
11+
dbimage:
12+
- postgres:13
13+
- postgres:12
14+
- postgres:11
15+
dbauth:
16+
- trust
17+
- md5
18+
- scram-sha-256
19+
swiftver:
20+
- 'swift:5.2'
21+
- 'swift:5.5'
22+
- 'swiftlang/swift:nightly-main'
23+
swiftos:
24+
- focal
25+
- amazonlinux2
26+
dependent:
27+
- fluent-postgres-driver
28+
container: ${{ format('{0}-{1}', matrix.swiftver, matrix.swiftos) }}
1729
runs-on: ubuntu-latest
18-
steps:
19-
- uses: actions/checkout@v1
20-
- run: swift test --enable-test-discovery --sanitize=thread
21-
postgres-kit_bionic:
22-
container:
23-
image: vapor/swift:5.2-bionic
30+
env:
31+
LOG_LEVEL: debug
32+
POSTGRES_HOSTNAME: 'psql-a'
33+
POSTGRES_HOSTNAME_A: 'psql-a'
34+
POSTGRES_HOSTNAME_B: 'psql-b'
2435
services:
25-
psql:
26-
image: postgres
27-
ports:
28-
- 5432:5432
36+
psql-a:
37+
image: ${{ matrix.dbimage }}
2938
env:
30-
POSTGRES_USER: vapor_username
31-
POSTGRES_DB: vapor_database
32-
POSTGRES_PASSWORD: vapor_password
33-
runs-on: ubuntu-latest
34-
steps:
35-
- uses: actions/checkout@v1
36-
- run: swift test --enable-test-discovery --sanitize=thread
37-
fluent-postgres-driver:
38-
container:
39-
image: vapor/swift:5.2
40-
services:
41-
postgres-a:
42-
image: postgres
43-
env:
44-
POSTGRES_USER: vapor_username
45-
POSTGRES_DB: vapor_database
46-
POSTGRES_PASSWORD: vapor_password
47-
postgres-b:
48-
image: postgres
39+
POSTGRES_USER: 'vapor_username'
40+
POSTGRES_DB: 'vapor_database'
41+
POSTGRES_PASSWORD: 'vapor_password'
42+
POSTGRES_HOST_AUTH_METHOD: ${{ matrix.dbauth }}
43+
POSTGRES_INITDB_ARGS: --auth-host=${{ matrix.dbauth }}
44+
psql-b:
45+
image: ${{ matrix.dbimage }}
4946
env:
50-
POSTGRES_USER: vapor_username
51-
POSTGRES_DB: vapor_database
52-
POSTGRES_PASSWORD: vapor_password
53-
runs-on: ubuntu-latest
47+
POSTGRES_USER: 'vapor_username'
48+
POSTGRES_DB: 'vapor_database'
49+
POSTGRES_PASSWORD: 'vapor_password'
50+
POSTGRES_HOST_AUTH_METHOD: ${{ matrix.dbauth }}
51+
POSTGRES_INITDB_ARGS: --auth-host=${{ matrix.dbauth }}
52+
steps:
53+
- name: Workaround SPM incompatibility with old Git on CentOS 7
54+
if: ${{ contains(matrix.swiftos, 'centos7') }}
55+
run: |
56+
yum install -y make libcurl-devel
57+
git clone https://github.com/git/git -bv2.28.0 --depth 1 && cd git
58+
make prefix=/usr -j all install NO_OPENSSL=1 NO_EXPAT=1 NO_TCLTK=1 NO_GETTEXT=1 NO_PERL=1
59+
- name: Check out package
60+
uses: actions/checkout@v2
61+
with:
62+
path: package
63+
- name: Check out dependent
64+
uses: actions/checkout@v2
65+
with:
66+
repository: vapor/${{ matrix.dependent }}
67+
path: dependent
68+
- name: Use local package
69+
run: swift package edit postgres-kit --path ../package
70+
working-directory: dependent
71+
- name: Run local tests with Thread Sanitizer
72+
run: swift test --enable-test-discovery --sanitize=thread
73+
working-directory: package
74+
- name: Run dependent tests with Thread Sanitizer
75+
run: swift test --enable-test-discovery --sanitize=thread
76+
working-directory: dependent
77+
78+
macos-plus-dependents:
79+
strategy:
80+
fail-fast: false
81+
matrix:
82+
xcode:
83+
- latest-stable
84+
- latest
85+
dbauth:
86+
- trust
87+
- md5
88+
- scram-sha-256
89+
formula:
90+
- postgresql@11
91+
- postgresql@12
92+
- postgresql@13
93+
dependent:
94+
- fluent-postgres-driver
95+
runs-on: macos-latest
96+
env:
97+
LOG_LEVEL: debug
98+
POSTGRES_HOSTNAME: 127.0.0.1
99+
POSTGRES_DB: postgres
100+
POSTGRES_HOST_AUTH_METHOD: ${{ matrix.dbauth }}
54101
steps:
55-
- run: git clone -b main https://github.com/vapor/fluent-postgres-driver.git
56-
working-directory: ./
57-
- run: swift package edit postgres-kit --revision ${{ github.sha }}
58-
working-directory: ./fluent-postgres-driver
59-
- run: swift test --enable-test-discovery --sanitize=thread
60-
working-directory: ./fluent-postgres-driver
61-
env:
62-
POSTGRES_HOSTNAME_A: postgres-a
63-
POSTGRES_HOSTNAME_B: postgres-b
102+
- name: Select latest available Xcode
103+
uses: maxim-lobanov/setup-xcode@v1
104+
with:
105+
xcode-version: ${{ matrix.xcode }}
106+
- name: Install Postgres, setup DB and auth, and wait for server start
107+
run: |
108+
export PATH="/usr/local/opt/${{ matrix.formula }}/bin:$PATH" PGDATA=/tmp/vapor-postgres-test
109+
brew install ${{ matrix.formula }}
110+
initdb --locale=C --auth-host ${{ matrix.dbauth }} -U vapor_username --pwfile=<(echo vapor_password)
111+
pg_ctl start --wait
112+
timeout-minutes: 2
113+
- name: Checkout code
114+
uses: actions/checkout@v2
115+
- name: Run local tests with Thread Sanitizer
116+
run: swift test --enable-test-discovery --sanitize=thread

‎Sources/PostgresKit/PostgresDataDecoder.swift

Lines changed: 71 additions & 106 deletions
Original file line numberDiff line numberDiff line change
@@ -12,16 +12,29 @@ public final class PostgresDataDecoder {
1212
public func decode<T>(_ type: T.Type, from data: PostgresData) throws -> T
1313
where T: Decodable
1414
{
15+
// If `T` can be converted directly, just do so.
1516
if let convertible = T.self as? PostgresDataConvertible.Type {
1617
guard let value = convertible.init(postgresData: data) else {
17-
throw DecodingError.typeMismatch(T.self, DecodingError.Context.init(
18+
throw DecodingError.typeMismatch(T.self, .init(
1819
codingPath: [],
19-
debugDescription: "Could not convert to \(T.self): \(data)"
20+
debugDescription: "Could not convert PostgreSQL data to \(T.self): \(data)"
2021
))
2122
}
2223
return value as! T
2324
} else {
24-
return try T.init(from: _Decoder(data: data, json: self.json))
25+
// Probably a Postgres array, JSON array/object, or enum type not using @Enum. See if it can be "unwrapped"
26+
// as a single-value decoding container, since this is much faster than attempting a JSON decode, or as an
27+
// array in the Postgres-native sense; this will handle "box" types such as `RawRepresentable` enums while
28+
// still allowing falling back to JSON.
29+
do {
30+
return try T.init(from: GiftBoxUnwrapDecoder(decoder: self, data: data))
31+
} catch DecodingError.dataCorrupted {
32+
// Couldn't unwrap it either. Fall back to attempting a JSON decode.
33+
guard let jsonData = data.jsonb ?? data.json else {
34+
throw Error.unexpectedDataType(data.type, expected: "jsonb/json")
35+
}
36+
return try self.json.decode(T.self, from: jsonData)
37+
}
2538
}
2639
}
2740

@@ -39,125 +52,77 @@ public final class PostgresDataDecoder {
3952
}
4053
}
4154

42-
final class _Decoder: Decoder {
43-
var codingPath: [CodingKey] {
44-
return []
45-
}
46-
47-
var userInfo: [CodingUserInfoKey : Any] {
48-
return [:]
49-
}
55+
private final class GiftBoxUnwrapDecoder: Decoder, SingleValueDecodingContainer {
56+
var codingPath: [CodingKey] { [] }
57+
var userInfo: [CodingUserInfoKey : Any] { [:] }
5058

59+
let dataDecoder: PostgresDataDecoder
5160
let data: PostgresData
52-
let json: PostgresJSONDecoder
5361

54-
init(data: PostgresData, json: PostgresJSONDecoder) {
62+
init(decoder: PostgresDataDecoder, data: PostgresData) {
63+
self.dataDecoder = decoder
5564
self.data = data
56-
self.json = json
65+
}
66+
67+
func container<Key>(keyedBy type: Key.Type) throws -> KeyedDecodingContainer<Key> where Key: CodingKey {
68+
throw DecodingError.dataCorruptedError(in: self, debugDescription: "Dictionary containers must be JSON-encoded")
5769
}
5870

5971
func unkeyedContainer() throws -> UnkeyedDecodingContainer {
60-
guard let data = self.data.array else {
61-
throw Error.unexpectedDataType(self.data.type, expected: "array")
72+
guard let array = self.data.array else {
73+
throw DecodingError.dataCorruptedError(in: self, debugDescription: "Non-natively typed arrays must be JSON-encoded")
6274
}
63-
return _UnkeyedDecoder(data: data, json: self.json)
64-
}
65-
66-
func container<Key>(
67-
keyedBy type: Key.Type
68-
) throws -> KeyedDecodingContainer<Key> where Key : CodingKey {
69-
let data: Data
70-
if let jsonb = self.data.jsonb {
71-
data = jsonb
72-
} else if let json = self.data.json {
73-
data = json
74-
} else {
75-
throw Error.unexpectedDataType(self.data.type, expected: "json")
75+
return ArrayContainer(data: array, dataDecoder: self.dataDecoder)
76+
}
77+
78+
struct ArrayContainer: UnkeyedDecodingContainer {
79+
let data: [PostgresData]
80+
let dataDecoder: PostgresDataDecoder
81+
var codingPath: [CodingKey] { [] }
82+
var count: Int? { self.data.count }
83+
var isAtEnd: Bool { self.currentIndex >= self.data.count }
84+
var currentIndex: Int = 0
85+
86+
mutating func decodeNil() throws -> Bool {
87+
// Do _not_ shorten this using `defer`, otherwise `currentIndex` is incorrectly incremented.
88+
if self.data[self.currentIndex].value == nil {
89+
self.currentIndex += 1
90+
return true
91+
}
92+
return false
93+
}
94+
95+
mutating func decode<T>(_ type: T.Type) throws -> T where T : Decodable {
96+
// Do _not_ shorten this using `defer`, otherwise `currentIndex` is incorrectly incremented.
97+
let result = try self.dataDecoder.decode(T.self, from: self.data[self.currentIndex])
98+
self.currentIndex += 1
99+
return result
100+
}
101+
102+
mutating func nestedContainer<NewKey: CodingKey>(keyedBy _: NewKey.Type) throws -> KeyedDecodingContainer<NewKey> {
103+
throw DecodingError.dataCorruptedError(in: self, debugDescription: "Data nesting is not supported")
104+
}
105+
106+
mutating func nestedUnkeyedContainer() throws -> UnkeyedDecodingContainer {
107+
throw DecodingError.dataCorruptedError(in: self, debugDescription: "Data nesting is not supported")
108+
}
109+
110+
mutating func superDecoder() throws -> Decoder {
111+
throw DecodingError.dataCorruptedError(in: self, debugDescription: "Data nesting is not supported")
76112
}
77-
return try self.json
78-
.decode(DecoderUnwrapper.self, from: data)
79-
.decoder.container(keyedBy: Key.self)
80113
}
81-
114+
82115
func singleValueContainer() throws -> SingleValueDecodingContainer {
83-
_ValueDecoder(data: self.data, json: self.json)
84-
}
85-
}
86-
87-
struct _UnkeyedDecoder: UnkeyedDecodingContainer {
88-
var count: Int? {
89-
self.data.count
90-
}
91-
92-
var isAtEnd: Bool {
93-
self.currentIndex == self.data.count
94-
}
95-
var currentIndex: Int = 0
96-
97-
let data: [PostgresData]
98-
let json: PostgresJSONDecoder
99-
var codingPath: [CodingKey] {
100-
[]
116+
return self
101117
}
102-
103-
mutating func decodeNil() throws -> Bool {
104-
defer { self.currentIndex += 1 }
105-
return self.data[self.currentIndex].value == nil
106-
}
107-
108-
mutating func decode<T>(_ type: T.Type) throws -> T where T : Decodable {
109-
defer { self.currentIndex += 1 }
110-
let data = self.data[self.currentIndex]
111-
return try PostgresDataDecoder(json: self.json).decode(T.self, from: data)
112-
}
113-
114-
mutating func nestedContainer<NestedKey>(
115-
keyedBy type: NestedKey.Type
116-
) throws -> KeyedDecodingContainer<NestedKey>
117-
where NestedKey : CodingKey
118-
{
119-
throw Error.nestingNotSupported
120-
}
121-
122-
mutating func nestedUnkeyedContainer() throws -> UnkeyedDecodingContainer {
123-
throw Error.nestingNotSupported
124-
}
125-
126-
mutating func superDecoder() throws -> Decoder {
127-
throw Error.nestingNotSupported
128-
}
129-
}
130-
131-
struct _ValueDecoder: SingleValueDecodingContainer {
132-
let data: PostgresData
133-
let json: PostgresJSONDecoder
134-
var codingPath: [CodingKey] {
135-
[]
136-
}
137-
118+
138119
func decodeNil() -> Bool {
139-
return self.data.value == nil
120+
self.data.value == nil
140121
}
141122

142123
func decode<T>(_ type: T.Type) throws -> T where T : Decodable {
143-
if let convertible = T.self as? PostgresDataConvertible.Type {
144-
guard let value = convertible.init(postgresData: data) else {
145-
throw DecodingError.typeMismatch(T.self, DecodingError.Context.init(
146-
codingPath: [],
147-
debugDescription: "Could not convert to \(T.self): \(data)"
148-
))
149-
}
150-
return value as! T
151-
} else {
152-
return try T.init(from: _Decoder(data: self.data, json: self.json))
153-
}
124+
// Recurse back into the data decoder, don't repeat its logic here.
125+
return try self.dataDecoder.decode(T.self, from: self.data)
154126
}
155127
}
156128
}
157-
158-
struct DecoderUnwrapper: Decodable {
159-
let decoder: Decoder
160-
init(from decoder: Decoder) {
161-
self.decoder = decoder
162-
}
163-
}

0 commit comments

Comments
 (0)