Trying to reduce boilercode

Using Swift
1

hello,

I'm migrating my app MrScrooge to vapor with swift, I'm in a really good point right now, but I've got a lot of boiler code, I'm using open-api autogenerated, and UUIDs for the ids. And I've got the following code almost identical in a lot of places:

guard let labelId = UUID(uuidString: input.path.labelId) else {
			return .badRequest(
				.init(
					body: .json(
						.init(
							message: "Label ID should be an UUID",
							code: ApiError.API10045.rawValue))))
		}

Since I need to validate the parameters are uuids.

I tried to create some macro that generates the full code or the return expression, but I wasn't unable to get it working. Since the return type from the open-api generation is different for every function, I cannot create a simple function.

Which Options I've got to reduce this code?

Thanks,
Jaume

2

Can you show a few more instances of the repetitive code, to make the duplicated structure apparent? Otherwise it’s hard to offer a suggestion here.

3

yes of course:

guard let ruleId = UUID(uuidString: input.path.ruleId) else {
			return .badRequest(
				.init(
					body: .json(
						.init(
							message: "Rule ID should be an UUID",
							code: ApiError.API10026.rawValue))))
		}

....
this comes the error from a switch
case .notUuid:
			return .badRequest(
				.init(
					body: .json(
						.init(
							message: "Parent ID not an UUID",
							code: ApiError.API10023.rawValue))))
...
// this one is slightly different, but can be splited into two diferent checks 
guard let groupOwnerId = UUID(uuidString: graphData.groupOwnerId),
			validGroupsId.contains(groupOwnerId)
		else {
			return .unauthorized(
				.init(
					body: .json(
						.init(
							message: "Not valid groupOwnerId",
							code: ApiError.API10004.rawValue,
							validGroupOwners: validGroupsId.map {
								$0.uuidString
							}))))

		}

At the end, is allways check the parameter is an uuid, and when not return an error. the structure is most of them the same, with a diferent text and diferent apiCode.

4

Some suggestions:

  1. Make "real errors" instead of your error codes like APIError.API10004
  2. Perhaps extract a function that raises those errors, and returns a UUID
  3. Add a common catch block that builds the .unauthorized() response from the caught error

Here's a rough idea:

protocol APIError {
    var errorCode: Int { get }
    var message: String { get }

    func toJSON() -> JSONBody // TODO: fix return type
}

extension APIError {
    func toJSON() -> JSONBody { // TODO: fix return type
        .json(.init(message: self.message, code: self.code))
    }
}

struct InvalidRuleID: APIError {
    var errorCode: Int { ApiError.API10026 }
    var message: String { "Rule ID should be an UUID" }
}

struct InvalidGroupID: APIError {
    var errorCode: Int { ApiError.API10004 }
    var message: String { "Not valid groupOwnerId" }

    let validGroupOwners: Set<UUID>

    func toJSON() -> JSONBody { // TODO: fix return type
        .json(.init(
            message: self.message,
            code: self.code,
            validGroupOwners: self.validGroupOwners.map(\.uuidString)
        ))
    }
}


func yourFunction(input: Input) -> Response {
    do {
        guard let ruleId = UUID(uuidString: input.path.ruleId) {
            throw InvalidRuleID()
        }
        
        guard let groupwnerId = UUID(uuidString: graphData.groupOwnerId),
              validGroupsId.contains(groupOwnerId) else {
            throw InvalidGroupID()
        }

        // ...
    } catch let error as InvalidGroupID { 
        return .unauthorized(.init(body: error.asJSON()))
    } catch let error as APIError { 
        return .badRequest(.init(body: error.asJSON()))
    }
5

Hello,

I'm not sure that will work. I can try, I will need to put some middleware like the following I expect:

struct ErrorHandlerMiddleware: AsyncMiddleware {
	func respond(to request: Request, chainingTo next: AsyncResponder) async throws -> Response
	{
		do {
			return try await next.respond(to: request)
		} catch {
			switch error {
			case let exception as Exception:
				print(exception.toJSON())
				throw error
			case let error as ServerError:
				if let decodingError = error.underlyingError as? DecodingError {
					throw Abort(
						.badRequest,
						reason: String(describing: decodingError))
				}
...

For what I saw the idea of OpenApi is to return the data as I'm doing.

Still thanks. I need to test more.

6

How about defining a set of validators (one for each check) and put them in an array? Then your middleware code just needs to apply the set of validators to the request. That should simplify the code a lot. For example:

// 1) Define the protocol
protocol RequestValidator {
    // RequestError is an enum
    func validate(request: Request) -> RequestError?
}

// 2) Define types conforming to above protocol, one for each check.
// ...

// 3) Define an array and put instances of the above type to the array.
var validators: [any RequestValidator] 

// 4) Modify middleware code to apply these validators to the request.
// ...

Defining one type for each check allows flexibility. If your code doesn't need it, you can replace the protocol with a type and implement the checks as instances.

I doubt if you can use macro to achieve code reuse in this case, because IMHO macro isn't the proper tool in this case.

7

every request have diferent input data, this means, I need to relate the request to the validator list, right?

8

You can define input data type as associated type of the protocol, though I'm not sure if you will be still able to use the protocol as type. Regarding how to relate request type to validator array, I guess primary associated type probably helps (I haven't used it myself but many people in the forum can help if you run into an issue in your further experiments).

9

ok, I'm already doing something similar, but right now only returning an enum, and manually calling it in the function (the sample of the case .notUUid is that)

10

Hey, I just realized one of the errors when I was doing my macro, and for the moment I've got at least one of the cases fixed, Still I will like to take a deeper look at the proposed solutions. But Fyi, I've got a macro like this:

public struct BasicBadRequest: ExpressionMacro {
	public static func expansion(
		of node: some SwiftSyntax.FreestandingMacroExpansionSyntax,
		in context: some SwiftSyntaxMacros.MacroExpansionContext
	) throws -> ExprSyntax {
		guard
			let message = node.arguments.first?.expression.as(
				StringLiteralExprSyntax.self)?.representedLiteralValue
		else {
			throw CustomError.message("First parameter should be an string")
		}

		guard
			let expression = node.arguments.dropFirst().first?.expression.as(
				MemberAccessExprSyntax.self)
		else {
			throw CustomError.message("Invalid code")
		}
		let text = expression.declName.baseName.text
		return """
					.badRequest(
						.init(
							body: .json(
								.init(
									message: "\(raw: message)",
									code: "\(raw: text)"))))
			"""
	}
}
11

I'd suggest you write out the repeated code first, to identify the parts that actually repeat, before trying to macro-ize the solution. As you might have already felt, macros can be pretty cumbersome to edit and refactor (as compared to just changing the code directly a couple times), so it'll slow down your iteration and discovery of an optimal solution.

I haven't seen anything here that really necessitates macros. Simple error throwing handles this well, from what I can tell.

12

I'm not sure that will work.

Can you elaborate on the issue?

I don't see why this needs to be in a middleware layer. It can eventually go there (if you need to reuse this among many places), but that seems premature.

For what I saw the idea of OpenApi is to return the data as I'm doing.

The solution I showed is "returning the data". What do you mean?

13

I didn't catch your JSONBody, and then returning that. Sorry, yes, that can be a good solution. (I will test next week)

I don't have it in 3 places, I've got in around 40 places similar code or more. (I'm by the code API10044, which means 44 different kind of errors returned) I think with that I can get a real idea of what I wish to generalize.

But my level of swift is not high (yet) of what allows and doesn't allow, also is the first think I'm using auto-generated code from open-api. (And the UUID parsing I feel really annoying that is not doing it)

Thanks!

P.D.: This is the type that open-api is generating as a return value:

            case badRequest(Operations.ApiLabels_update.Output.BadRequest)
            /// The associated value of the enum case if `self` is `.badRequest`.
            ///
            /// - Throws: An error if `self` is not `.badRequest`.
            /// - SeeAlso: `.badRequest`.
            internal var badRequest: Operations.ApiLabels_update.Output.BadRequest {
                get throws {
                    switch self {
                    case let .badRequest(response):
                        return response
                    default:
                        try throwUnexpectedResponseStatus(
                            expectedStatus: "badRequest",
                            response: self
                        )
                    }
                }
            }
...
internal struct BadRequest: Sendable, Hashable {
                /// - Remark: Generated from `#/paths/labels/{labelId}/PUT/responses/400/content`.
                internal enum Body: Sendable, Hashable {
                    /// - Remark: Generated from `#/paths/labels/{labelId}/PUT/responses/400/content/application\/json`.
                    case json(Components.Schemas._Error)
                    /// The associated value of the enum case if `self` is `.json`.
                    ///
                    /// - Throws: An error if `self` is not `.json`.
                    /// - SeeAlso: `.json`.
                    internal var json: Components.Schemas._Error {
                        get throws {
                            switch self {
                            case let .json(body):
                                return body
                            }
                        }
                    }
                }
                /// Received HTTP response body
                internal var body: Operations.ApiLabels_update.Output.BadRequest.Body
                /// Creates a new `BadRequest`.
                ///
                /// - Parameters:
                ///   - body: Received HTTP response body
                internal init(body: Operations.ApiLabels_update.Output.BadRequest.Body) {
                    self.body = body
                }
            }

As you can see is generating unique types per endpoint all the time.

14

If you really want to keep experimenting with macros I highly recommend keeping the Swift AST Explorer available. This tool has been super helpful for me when learning to work with swift-syntax.

I also recommend checking out the swift-syntax repo for all the macro examples and sample code. The Observable macro from the main swift repo is also all open-source.

I don't have much context on open-api to have an opinion whether or not macros are the best solution or not. What you are proposing sounds like something that other engineers would have experienced. Have there been discussions in the open-api community about a pattern for this specific operation?

15

Hey thanks,

I will take a look. I didn't continue with the macro by itself, simply that I gived another try (I already had the code), and then I realise the error. Also, from what I see with the types, I really don't see another option, apart from extending ALL the types openapi generate with my custom protocols to be able to group everything that is similar. (It's not something that seems gratefull to do, at least for me)

I started with this refactor like 3 month ago, But I pushed a lot. And the small tutorial of open-api-generator is what I'm using.

Thanks to everybody.

16

Is there some common protocol these generated types conform to? Perhaps generics cover it.

Are there any other prominent code bases that use this OpenAPI library, that you could compare against?

IMHO it's crazy for a library to be designed in a way that requires users to write macros to use it. There's almost certainly a simpler and more direct way to handle it.

17

I'm using this with the vapor extension:

It's to generate a server compliant with the open-api file I create. That way I can generate also the types for the pronted.

Yeah, I agree, at the begining I didn't realise it, but now is... I'm copy pasting everything, but I don't see any common protocol or anything to reuse it.