Stl 1821 erica api v.2

[VictorDelCampo]

Short Description

• Added v2 version leaned to FastAPI structure (v1 adapted to that structure).

Changes

• Version 01 restructured, leaned to FastAPI structure
• Create new version 02 with only "tax_offices" implemented
• Other requests will communicate with the job queue and therefore for now NotImplementedError, but with an exception catch for producing a dummy response in accordance with the specification
• There are 2 preconfigured docs urls that can be accessed as soon as the erica service is started:
  -- Swagger UI served at /docs
  -- ReDoc UI served at /redoc

Feedback

• I am not really happy with the big class/variable names, specially for the unlock_code requests.
• But i wanted to maintain all params/variables passed in the v1 requests
• The success responses should be defined when the actual implementation is done, right?
• About the documentation, are the docs urls what was meant in the ticket?
• Grundsteuer folder in v2 is empty. What about informing the team? How should it be done and what is to say?

How to review this Pull Request

Link to Confluence (internal)

[VictorDelCampo]

@JulianRoesner

• Variable "include_elster_response" removed from all payload models.
• Variable "ttlInMinutes" unchanged, using camel case would means the actual name of the variable in the request payload would differ from your Tech Spec
• Response models for v2 were added but for the get_job-requests the default 422 ValidationError from openAPI will be displayed in the docs. Actually it is not possible that this error occurs since if we don't pass a request_id then we would be trying to access the API of the POST request and we would get a HTTp 405 Method not allowed. There is a way to completely removed the default 422 from openAPI but this would mean inherting from FastAPI and rewriting the method openapi (How to disable the default 422 doc fastapi/fastapi#3424)
• Creating response models for v1 would mean updating all API resources so that we return JSONRespones and not HTTPException, but you said, v2 would be important and v1 only if possible and easy?

If you find all changes OK, just approve and I would merge to main and let Frede now about our changes.

[JulianRoesner]

Very nice. I just have a question about the response_models and I am wondering if there would be a better way to structure the erica_input file

[punknoir101]

Why we don't use the standard way to versioning APIs? For Fast API there is fast api versioning package which uses annotations (The more common way, I would say.)

here:
app = FastAPI(title="Erica Api")
app = VersionedFastAPI(app, default_api_version=(1))

and annotation like this:
@version(1, 2)

in routes

swagger:

[JulianRoesner]

Why we don't use the standard way to versioning APIs?

Can you elaborate what you mean with the standard way?

I like the fast api versioning package. However, we could only use it for version 2 onwards as this would change the routes

[VictorDelCampo]

I'v just made an implementation of that library in our erica and looks quite nice! We can adapt how the versions are formatted, including a prefix, like this:

app = VersionedFastAPI(app, version_format='{major}', prefix_format='/0{major}')

And then we only use the major version decorator:
@version(2)

This way we can also update the version markers for 01

My concerns on this library (following our specification for adding a new external library):

• last release was 1 year ago
• last commit 6 months ago
• 20 open issues (5 of them from 2020 and rest from 2021 but 2 from 2022)
• 4 open PR (all of them from 2021)

I have just pushed my implementation with that library. One BIG advantage of using the library is that we have the docs in Swagger in the corresponding version urls:
/01/docs
/02/docs

[JulianRoesner]

This way we can also update the version markers for 01

Nice.

My concerns on this library(following our specification for adding a new external library):
last release was 1 year ago
last commit 6 months ago
20 open issues (5 of them from 2020 and rest from 2021 but 2 from 2022)
4 open PR (all of them from 2021)

Thanks for checking. Yes I think I agree with your concerns. Apparently, it started more as a proof of concept. Have a look at this article. It mentions that there might be problems with custom error handling. Maybe we should have a look into that.
Also they have an issue open asking if the project is still active.

[VictorDelCampo]

The only advantage of using this library would be, for me, having by default different docs urls for each version. FastAPI offers the possibility to change both urls (Swagger and Redoc) but we would need two different FastAPI instances in which we could separately add the routers of each version and then setting specific docs url, like stated here. But the question would be, do we want separate docs urls?

Using this library, we still need to decorate every API method we define or we have defined. This was't necessary for the prefix versioning way.

Regarding the problems with custom error handling, it would be a stopper for us if we were already using those, but as far as I know, we don't. Nevertheless the project has on github many open issues and most of them differ from each other. In addition the unanswered question of the project being active or not for almost a month now.

In my opinion I would avoid using this library. It looks inactive since October 2021:

WDYT?

[JulianRoesner]

We can already see what is outdated fast by checking the folder of the api, since we moved 01 to folder v1 and the new 02 to folder v2.

Yes, I thought so.

I would then revert the changes I made when introducing FastAPI-versioning and go on with the prefix technique within the API router?

That's what I would say. However, we probably should rename the version two to follow the standard @punknoir101 mentioned?

[VictorDelCampo]

Meaning 2.0 and not 02?

[punknoir101]

It's about the open api standard and the api documentation - if you do it "your" way it will work, but it's not properly documented - the swagger.json/open-api.json file would be wrong then. So other services that will follow open api standard would have problems to consume different versions. Open Api json is the contract and should be properly documented!

[VictorDelCampo]

Not really getting your point. The open api standard has a field within called "openapi", which is a required string and " MUST be the semantic version number of the OpenAPI Specification version that the OpenAPI document uses."

And it follows "This is not related to the API info.version string."

That field "version" within the object "info" of the specification is just a simple string. And this value can be set through the FastAPI instance. If we don't use the FastAPI-Versioning we could just create 1 FastAPI instance for each API version we have and defined the version in the format we want.

WDYT?

[JulianRoesner]

Sorry to open that up again. While looking for resources for another discussion I came upon this article and this one that are both talking about including the api version in the uri and using v1 or v2 for that.

The second article was also the origin of the v1 in our code base. So, as far as I see it, this would be a standard way to expose our version through the url and then we could still version the whole api through FastApi's spec.

@punknoir101 @VictorDelCampo

[JulianRoesner]

LGTM! We should have a check that everything works on staging as part of QA.

[VictorDelCampo]

@punknoir101 just waiting for your approval too :)

[punknoir101]

i.O.

[VictorDelCampo]

@JulianRoesner I added an enum for the "Status" and modified the base class of the response model when getting a job from the queue

[JulianRoesner]

Mostly some stuff that I did not specify correctly. If you do not agree, let's talk.

Great idea with the enum

[sonarqubecloud]

Kudos, SonarCloud Quality Gate passed!   

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

No Coverage information
0.0% Duplication