Transport Agnostic Errors / EventStream errors - consolidate error handling #216
Conversation
alextwoods
changed the title
| data.top_level = map['TopLevel'] | ||
| data.nested = (ComplexNestedErrorData.parse(map['Nested']) unless map['Nested'].nil?) | ||
| data | ||
| unless body.empty? |
There was a problem hiding this comment.
Even if body is empty, wouldn't we parse an empty map easily here?
There was a problem hiding this comment.
I think in at least some cases we used to fail on this (xml?) - it also means you can skip any parsing + checking for each field in the map when you already know you don't need to do any more parsing. We used to just return the data here immediately, so I think this new logic is equivalent to that + the error class wrapper.
codegen/projections/white_label/lib/white_label/event_stream.rb
Outdated
Show resolved
Hide resolved
| @@ -1,25 +0,0 @@ | |||
| # frozen_string_literal: true | |||
There was a problem hiding this comment.
I think there's rbs for this file? Check to see if any other rbs needs updates.
There was a problem hiding this comment.
Good call removed and checked all other rbs.
| @@ -23,12 +23,20 @@ class ApiClientError < ApiError; end | |||
| class ApiServerError < ApiError; end | |||
|
|
|||
| class ModeledError < ApiClientError; end | |||
|
|
|||
| module Parsers | |||
There was a problem hiding this comment.
I think this should be under TestParsers, next to TestErrors? (Errors and Parsers are siblings right?)
There was a problem hiding this comment.
Was trying to avoid extra namespaces. The module arrangement doesn't really matter here, though I guess technically it doesn't reflect what we generate. To be fully accurate we would need:
module TestService
module Errors
class ModeledError < ApiClientError; end # and all the other ones
end
module Parsers
class ModeledError; end
end
end
But it didn't really seem worth it to create more modules/nest more deeply here.
There was a problem hiding this comment.
In general I didn't like that we pollute the test namespace. There must be a fix.. maybe we should make a unit test service somehow? I guess like an API helper.
Description of changes:
Previously the generated
Errorsmodule was specific to the HTTP Transport. This PR makes the generated errors transport agnostic, allowing errors classes to be used as either operation response errors (with or without http transports) and event errors. This required moving the creation of events into the parsers, which do have the transport specific knowledge and the the knowledge of which error class they map to.Consolidates the error handling for event stream errors to a single handler for consistency with the SDK specification. Adds modeled event stream errors to new generated Errors::EventStream module. Note - these must be in a separate namespace as the error structures may also be the target of operation errors.
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.