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 ATError Object Generator #112

Merged
merged 3 commits into from
Nov 27, 2024
Merged

Add ATError Object Generator #112

merged 3 commits into from
Nov 27, 2024

Conversation

drasticactions
Copy link
Owner

Inspired by @PassiveModding's PR for #111, this generates ATError objects based on the error object within the lexicon for a given endpoint. This is just hacked together to expose the idea of it (It could be expanded to use details if available, or be included in the constructor documentation as possible error types) but it should now make it possible to switch inside of the error flow for a given error, rather than inspecting it yourself.

スクリーンショット 2024-11-27 0 16 57

@PassiveModding
Copy link
Contributor

the aim of #111 was more to have a strongly typed reference such that I can respond with these from an api controller as a pds would.
an example:

    [HttpPost("com.atproto.server.createAccount")]
    public async Task<IActionResult> CreateAccount([FromBody] CreateAccountInput request)
    {
        try
        {
            var config = await _configRepository.GetConfigAsync();
            
            // directs user back to 'invite code' page in appview
            if (string.IsNullOrWhiteSpace(request.InviteCode))
            {
                // alternatively something akin to return ATErrorResponse(new InvalidInviteCodeError(StatusCode.BadRequest, "missing or invalid invite code"));
                return ATErrorResponse(Com.Atproto.Server.CreateAccount.Errors.InvalidInviteCode, "missing or invalid invite code");
            }
            ...


    public static IActionResult ATErrorResponse<T>(T enumError, string message) where T : Enum
    {
        return new ErrorResponseResult<T>(enumError, message, HttpStatusCode.BadRequest);
    }
    
    public class ErrorResponseResult<T> : ObjectResult where T : Enum
    {
        public ErrorResponseResult(T enumError, string message, HttpStatusCode statusCode) : base(new { error = enumError.ToString(), message = message })
        {
            StatusCode = (int)statusCode;
        }
    }

Personally my thoughts on the approach you have taken would be that you should still be able to construct an error type with the detail.Error field pre-populated

new InvalidInviteCodeError(statusCode, message)

namespace  FishyFlip.Lexicon
{
    /// <summary>
    /// FishyFlip ATError for InvalidInviteCode.
    /// </summary>
    public class InvalidInviteCodeError : FishyFlip.Models.ATError
    {
        public const string ErrorType = "InvalidInviteCode";
        
        public InvalidInviteCodeError(int statusCode, ErrorDetail detail) : base(statusCode, detail)
        {
        }
        
        public InvalidInviteCodeError(int statusCode, string message) : base(statusCode, new ErrorDetail(ErrorType, message))
        {
        }
    }
}

@PassiveModding
Copy link
Contributor

I also think it would be nice to preserve the namespace and descriptions associated with the error responses but given that multiple routes can have the same error type it would cause issues in ATErrorGenerator.g.cs in it's current state since there would be multiple classes for a given errorType. Perhaps they could just be included as part of the summary xmldoc

@drasticactions
Copy link
Owner Author

The ATError object should already be returning the detail error message as part of ErrorDetail. The descriptions there shouldn't be necessary in that case, and adding them after the fact may be incorrect as to the reason for the error, so I don't feed comfortable doing that.

For having the errors themselves be encapsulated within the namespace (E.g having multiple of them) I'm not against it, but I'm trying to think how it would be useful in practice. Generating them would be straightforward, and to get the "correct" one would be to get the response URI, parse it for the Id and switch on that along with the error code to return the error class object (or the generic one if it doesn't exist).

Remember that the only difference in any of these objects is the value of ErrorDetail.Error. There is no other information, that I'm aware of, being returned that is otherwise missing. Checking the Lexicon, there are instances where there different areas are returning the same known error:

image

The object returned from the endpoint is the same between them all. You know the context of the error because you're getting it as part of a call to ATProto, so you know the endpoint you're calling. For that reason, I'm unsure if having this be two or three separate objects adds value...

That said, having it be fully namespace could add in discoverability. Since then if you wanted to know the known error types for a given endpoint, you could maybe type out FishyFlip.Lexicon.Com.Atproto.Repo.GetRecord.Errors to see the known errors and switch on them directly. It adds complexity, but makes it clear that these are expected errors and removes ambiguity.

I'll play around with it some more.

@PassiveModding
Copy link
Contributor

InvalidSwap has different descriptions in com.atproto.repo.applyWrites and com.atproto.repo.createRecord and no description in com.atproto.repo.putRecord and com.atproto.repo.deleteRecord. Though that appears to be the only instance that mixes different description values

@PassiveModding
Copy link
Contributor

PassiveModding commented Nov 27, 2024

And you're correct in the fact ATError.ErrorDetail would include it, if sourced from an error event but from the perspective of
"I want to construct and return these errors from an api", having a statically typed way to create the ErrorDetail class instance would be useful. I think continue with the current implementation but also generate instances ErrorDetail under the namespaces for each class.
image
vs
image

public class InvalidInviteCodeErrorDetail : FishyFlip.Models.ErrorDetail
{
    public InvalidInviteCodeErrorDetail(string message) : base("InvalidInviteCode", message)
    {
    }
}

@drasticactions
Copy link
Owner Author

The description values do have use for the Endpoint classes when stating the list of errors and what could be expected, so they could be added to the top constructor of the given main methods pointing to these exceptions.

I see your point for using ErrorDetail and that usecase but IMO I view these result objects as from the API rather than something you construct yourself and pass downstream to your consumers. In past versions I had sealed and internalized many of the component classes to try and enforce "This is from ATProtocol, don't extend it or make your own!" I really want to be clear that it's coming from you calling ATProtocol and it returning that object and/or error. I opened it up for this version as I saw some complaints that it prohibited mocks for tests, but I'm kinda operating on that thought process.

For example, in your use-case of the Invite Code. For a front-end client, you would be checking that with client-side validation first most likely to prevent that request from hitting your server. Your backend code could also validate it before calling ATProtocol, but you would probably want to handle that error yourself to make it clear that something went wrong in your application to get to that state, rather than return an object from this library. Or you could not validate that value in the backend, let it get processed by ATProtocol, which would return the error object, that you then pass back to your client application to handle.

@drasticactions drasticactions merged commit 286ac5a into develop Nov 27, 2024
3 checks passed
@drasticactions drasticactions deleted the error-generator branch November 27, 2024 10:36
drasticactions added a commit that referenced this pull request Dec 27, 2024
* Add missing parameters to getAuthorFeed

* Don't set ATError object when logging

* Add AspectRatio to ImageView

* Start sourcegen

* Update

* Update Tools

* Update

* Remove usings, since it's in global

* Try again

* update

* Update

* Update

* Update

* Update

* Update

* Update

* Almost...

* Update

* Update

* Update logging-in.md

* Add codeflow to readme

* Update...

* Expose the Cursor on the NotificationCollection

* Almost... maybe...

* Firehose

* Update

* The big one

* Update

* Update...

* Fix Readme

* Start refactoring...

* More types

* More cleanup

* More modding...

* More types...

* Closer...

* Nearly...

* Fun!

* First run!

* Tests and More

* Add Tests for Endpoints

* Add class entries

* Trying to generate helpers...

* More generated Parameters

* And helper methods!

* Update Generated Types with less aggressive Null values

Also fix JSON ATObject type to allow fallback

* Update Docs, bump ATProto again...

* Revert...

* Revert...

* Revert...

* Add parameter constructors...

* Update docs

* More docs

* Update docs

* Adjust tests (???)

* Remove deprecated classes and methods

* Remove FishyFlipDX, Replace FishyFlip with Source Gen Bits (#104)

+semver: breaking

* Add docs, bump to net9.0 (#105)

* Add docs, bump to net9.0

* Bump to net9

* Fix ImportRepo, add OpenGraphParser (#106)

* Fix ImportRepo

* Add OpenGraphParser

* Cleanup + Fix WASM support  (#108)

* Order by classname

* Update

* small cleanup

* Update

* Use SupportsAutomaticDecompression for setting AutomaticDecompression, add WASM site back

* Add WhiteWind Generator, add GetRecord (#110)

* Add ATError Object Generator (#112)

* Add Error Generator

* Add Possible Errors

* Warning cleanup

* Add ToString override for ATError (#113)

* Fix GetBlob to return byte[] (#114)

* Resolve Repo DIDs and Handles independently (#115)

* Start refactoring URL

* Update

* Fix tests

* Fix Facet Parsing (#116)

* Fix Facet Parsing

* Remove DistinctBy

* Reintroduce bskycli (#117)

* Add bskycli

* Image Aspect Ratio

* Update

* Update

* Update readme

* Update

* Fix Windows script

* Bump ATProtocol, Bind ATFile Lexicon support (#118)

* Bump atprotocol

* Add ATFile support

* Add build_binding script (#119)

* Update PasswordAuth with AuthenticateWithPasswordResultAsync (#120)

* Update PasswordAuth with AuthenticateWithPasswordResultAsync

* Really???

* Use public.api.bsky.app for unauthed sessions by default (#121)

* Add 2FA Auth Token support for Password Auth (#122)

* Bump dependencies (#123)

* Add Post helper for PostView (#124)

* Bump dependencies (#125)

* Resolve users PDS before authentication (#126)

* Add Pastesphere support (#127)

* Add AniBlue support (#128)

* Bump Bindings (#129)

* Add community lexicons (#130)

* Add DidDoc helpers (#131)

* patch: Special case booleans (#132)

* Bump bindings (#133)

* Override ATUri/ATIdentifier Equals for proper checking (#134)

* Bump bindings

* Override Equals

* Add support for setting Labels / Fixes Chat (#135)

* Update SourceGen for Labels

* Update for labels

* Fix chat too

---------

Co-authored-by: Thomas May <[email protected]>
Co-authored-by: Kevin Gosse <[email protected]>
Co-authored-by: Matt Brailsford <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants