refactor: Improve the HTTP API implementation #382

fredcarle · 2022-04-27T03:18:34Z

Resolves #372
Resolves #367

Creating this draft PR to get a bit of feedback. Let me know what you think.

The server test functions are there mainly for code coverage and are kinda useless at the moment. Will be useful eventually if the server instantiation becomes more involved.

Let me know if you find any value in unit testing all the handler functions. I find it not so worth it for the trivial ones and those that use the db connection need to be decoupled (create a service interface to be able to mock the DB methods).

codecov · 2022-04-27T04:21:05Z

Codecov Report

Merging #382 (56862b5) into develop (dbbe282) will decrease coverage by 1.13%.
The diff coverage is 36.62%.

@@             Coverage Diff             @@
##           develop     #382      +/-   ##
===========================================
- Coverage    65.66%   64.53%   -1.14%     
===========================================
  Files           80       86       +6     
  Lines         9481     9866     +385     
===========================================
+ Hits          6226     6367     +141     
- Misses        2624     2867     +243     
- Partials       631      632       +1

Impacted Files	Coverage Δ
api/http/handlerfuncs.go	`0.81% <0.81%> (ø)`
api/http/errors.go	`100.00% <100.00%> (ø)`
api/http/handler.go	`100.00% <100.00%> (ø)`
api/http/logger.go	`100.00% <100.00%> (ø)`
api/http/router.go	`100.00% <100.00%> (ø)`
api/http/server.go	`100.00% <100.00%> (ø)`

shahzadlone

Awesome work @fredcarle. Just a few minor things:

Just a bit concerned about not handling inconsistencies between different GQL clients (more detail here: Defra Server GQL Endpoint Bugs Out Using GQL Clients #367)
We use the Git Rebase Flow vs Git Merge Flow. Just to avoid those merge commit pollutions from develop branch into the feature branch and also to keep a cleaner history for the reviewers (happy to go over my flow if you'd like =D).
You should update the README.md of the repo which currently says:

Once you've confirmed your node is running correctly, if you're using the GraphiQL client to interact with the database, then make sure you set the GraphQL Endpoint to http://localhost:9181/graphql and the Method to GET

Add your name in the README.md under the Contributors section. WOOOT WOOOOT!!!!!!
You can use resolving words in the PR description to auto link the PR to an issue so it closes on merge (Linking a pull request to an issue). So your first line can be something like "Resolves Refactor: HTTP API #372".
A slightly more descriptive PR title, where the first word is an action of some sort (here is a nice list: https://gist.github.com/scmx/411f6fea4ee3832806720d536a7d5d8f). btw not sure if you stumbled across these yet or not but the subset of conventional commit labels we use can be found here: https://github.com/sourcenetwork/defradb/blob/develop/tools/configs/chglog/config.yml.

api/http/handler.go

shahzadlone · 2022-04-27T10:41:55Z

api/http/handler.go

+	if c.req.Method == "GET" {
+		query = c.req.URL.Query().Get("query")
+	} else {
+		body, err := io.ReadAll(c.req.Body)
+		if err != nil {
+			http.Error(c.res, err.Error(), http.StatusBadRequest)
+			return
+		}
+		query = string(body)
+	}
+
+	if query == "" {
+		http.Error(c.res, "missing GraphQL query", http.StatusBadRequest)
+		return
+	}


I tested it with Altair and Insomnia,

This keeps Altair broken for POST requests and Insomnia broken for GET requests.

I made a more detailed comment on why and gave a suggestion here: #367

AndrewSisley

Code reads really quite nicely - thanks! Quite comment heavy, but a lot are just questions and I'm guessing a fair few would have been resolved before converting to a full-PR. Glad you are getting stuck in already :)

AndrewSisley · 2022-04-27T14:50:59Z

api/http/handler.go

+
+// HandlerConfig holds the handler configurable parameters
+type HandlerConfig struct {
+	Logger logging.Logger


suggestion: It feels strange to me that a 'config' object contains a configured instance of something (here Logger) - suggest this object hosts log config alongside any http related stuff and whatever consumes this config also configures the logger.

Be careful with this as you'll need to handle config for all defra-packages that this lib consumes.

Fair point. I'm not 100% sure I understand your suggestion but I'll give it a try and you can tell me if that's what you meant.

Was thinking the config object should be something similar to the below:

type HandlerConfig struct { loggingConfig { Level:... EnableStackTrace:... .... }, httpConfig { port:... ... }, dbConfig { Address:..., Badger:... }, }

A continuation of my comments elsewhere, I dont think its necessary to pass in the logger to the handler package, and we certainly wouldnt need to define the loglevel/stacktrace/config info for the logger within this config obj, as the logger stuff is defined elsewhere (at the entry point, eg CLI package).

AndrewSisley · 2022-04-27T14:52:21Z

api/api.go

+)
+
+// NewHTTPServer returns a Server loaded with the HTTP API Handler.
+func NewHTTPServer(db client.DB, c ...*http.HandlerConfig) *http.Server {


question: Why would you have many configs?

You wouldn't. It's a pattern used to make the config optional.

Would suggest using a type that only permits a single config value then, or use 2 functions (one with config, one without). Silently dropping configs 1..n might not be so fun for consumers.

Im guilty of also using this pattern in Go to create an "optional" function argument, im 50/50 on if it needs to be split into two funcs, one with no config, and one with a single config.

I have seen this pattern in gRPC and a few other Go projects too. This was also one of the recommendations in the "100 Go Mistakes and How to Avoid Them" book. I think it's fine to have one function.

AndrewSisley · 2022-04-27T14:56:11Z

api/api_test.go

+func TestServer(t *testing.T) {
+	s := NewHTTPServer(nil, nil)
+
+	if ok := assert.NotNil(t, s); ok {


question: why the if? And on further thought, is the if ok := assert.NotNil(t, s); ok { worth it when calling assert.Error(t, s.Listen(":303000")) if s is null should provide a very clear error anyway?

Because assert doesn't return or break the process if s is nil. Also, calling s.Listen with s being nil will pollute the terminal with a stack trace while checking with assert.NotNil will give a clean and concise error message.

this is an issue the plagues most of the test packages. I accidentally started using assert package from that repo but it turns out they have a more appropriate package called "require" which will automatically fail the test at that point in code without any handling on our part.

So assert.NotNil becomes require.NotNil, etc

:) I remembered that convo and couldnt remember/find-where-we-fixed-this. Might be good to go through the hassle of deciding to migrate (and then migrating) sooner rather than later.

AndrewSisley · 2022-04-27T15:04:04Z

api/api.go

+// by the Apache License, Version 2.0, included in the file
+// licenses/APL.txt.
+
+package api


question: What do you see this package containing? It's reason-to-be seems unclear at the moment (would suggest package-level docs when converting from draft-PR to normal-PR)

Not sure yet. Maybe the GRPC API could be here too. Maybe a messaging service for queues. Maybe we could have a discussion about it.

What's the benefit of having the package exist now?

To be honest, what initiated this change on my side was seeing

import ( api "github.com/sourcenetwork/defradb/api/http" )

I'm not a fan of "importing as" especially if the package name has no conflict. So I resorted to create the api package to return the api we want. Maybe I should have just removed the api name and use http to call NewServer. What do you think? This way we could avoid the api package and the import as.

I think this was a preemptive organization descision in case we has differnt transports/protocols for accessing the API beyond HTTP. But, for example, the gRPC API is defined/instanciated elsewhere

Would vote for dropping the API package now (keeping it simple), and then add it when we need it. Cost to add is low, and it makes things easier to understand in the meantime

AndrewSisley · 2022-04-27T15:10:02Z

api/http/handler.go

+
+	h.logger = defaultLogger()
+
+	h.setRoutes()


todo: this looks like a bug? If you provide a logger via c then setRoutes is never called. Would suggest replacing the if-early-return with an if-else, otherwise similar issues will be easily introduced (if solved e.g. by adding a h.setRoutes call within the if)

Good catch. It was indeed a bug. Fixed

AndrewSisley · 2022-04-27T15:44:49Z

api/http/handler.go

+	}
+}
+
+func getBlock(c *context) {


thought: Didnt realize we touched so many internal concepts here! This looks largely like a copy-paste from the old code, but please dont shy away from cleaning this up a tad if you feel like it (but really no pressure, might be worth creating an issue though if you dont) - really don't think the http package should be going anywhere near cid instantiation and protobuf....

I was thinking the same. I wanted to refactor the general http package before touching the handler functions so I thought I would create a separate issue to take care of that. Like phase 1: http package architecture, phase 2: refactoring handlers.

agreed on approach, structure then handlers:)

AndrewSisley · 2022-04-27T15:49:28Z

api/http/handler_test.go

+	"github.com/stretchr/testify/assert"
+)
+
+func TestHandler(t *testing.T) {


nitpick: took me a few seconds to realise this wasnt a test utility, but actually a test. Might be worth renaming.

Also (more seriously) the name implies to me that it is and will be the only test for the handler (with default config) - might be worth a rethink here as you get nearer non-draft PR status

Fair enough. I'll rename to be more specific.

AndrewSisley · 2022-04-27T15:54:21Z

api/http/handler.go

+		db: db,
+	}
+
+	if c != nil {


suggestion: I am very very against passing nil into a function, I didn't realize you were planning on doing this until I saw the tests. Suggest removing this if c != nil { check and dont pass nil (if this is why c is a pointer, would suggest de-pointering the param too).

A more idiomatic way of doing this is to have NewHandler(db client.DB) and NewHandlerWithConfig(db client.DB, c config) or just NewHandler(c config) with db client.DB passed in the config. Which one do you prefer?

It's not uncommon in Go to pass nil to a function that behaves differently depending on nil or not nil. Probably not using it in the most appropriate way here though. However, if our preference is to avoid that, fine by me.

Lots of people like to do it in other languages that permit this too, and every now and then users get null point exceptions at runtime... Is easy enough to avoid these, and if you want the occasional optional param you can easily wrap it (even easier with generics):

type OptionalFoo struct { Foo Foo, HasValue bool }

That way everyone is aware that it can be nil, instead of having to worry about the validity of every pointer in the codebase. A few newer langs have this built in (rust/C#/I-think-typescript-but-we-might-have-done-that-manually-at-my-old-place).

I'll reiterate Fred's point that its common to have pointers as arguments (for several reasons) and then having appropriate nil checks within the funcs. Moreover, the compiler is able to make several optimizations if it knows there are specific nil checks within a func, and ensure that all future pointer access is optimized without the need for runtime nil checks.

Not fussed about performance here (is almost always peanuts), but the needless risk of trying to call a member on a nil pointer.

AndrewSisley · 2022-04-27T15:56:55Z

api/http/handler_test.go

+
+}
+
+func TestHandlerWithConfig(t *testing.T) {


nitpick: Similar points to the comment RE the other test function's name, this function also suggests it should be the only test for any/all config and whilst maybe a bit hard to do atm as config is fairly empty, would suggest the name reflects the contents of the config under test

AndrewSisley · 2022-04-27T16:00:19Z

api/http/server.go

+}
+
+// NewServer instantiated a new server.
+func NewServer() *Server {


question: Why return a pointer here?

Habit of constructors in Go generally returning a pointer. Here in doesn't have to be.

Happy as is, was just curious

fredcarle · 2022-04-27T18:12:25Z

Awesome work @fredcarle. Just a few minor things:

Thanks Shahzad!

Just a bit concerned about not handling inconsistencies between different GQL clients (more detail here: Defra Server GQL Endpoint Bugs Out Using GQL Clients #367)

I'll investigate further and give those GQL clients a try myself so I can get a feel for it.

We use the Git Rebase Flow vs Git Merge Flow. Just to avoid those merge commit pollutions from develop branch into the feature branch and also to keep a cleaner history for the reviewers (happy to go over my flow if you'd like =D).

Oh yes John had mentioned that and I forgot to do it. I fixed that but I'd still like to see your flow. Let me know when that works for you.

You should update the README.md of the repo which currently says:

Once you've confirmed your node is running correctly, if you're using the GraphiQL client to interact with the database, then make sure you set the GraphQL Endpoint to http://localhost:9181/graphql and the Method to GET

Done!

Add your name in the README.md under the Contributors section. WOOOT WOOOOT!!!!!!

Yes!!

You can use resolving words in the PR description to auto link the PR to an issue so it closes on merge (Linking a pull request to an issue). So your first line can be something like "Resolves Refactor: HTTP API #372".

I edited the initial comment to include resolves

A slightly more descriptive PR title, where the first word is an action of some sort (here is a nice list: https://gist.github.com/scmx/411f6fea4ee3832806720d536a7d5d8f). btw not sure if you stumbled across these yet or not but the subset of conventional commit labels we use can be found here: https://github.com/sourcenetwork/defradb/blob/develop/tools/configs/chglog/config.yml.

I updated the title. Let me know if that fits the bill better.

fredcarle · 2022-04-27T22:15:46Z

@AndrewSisley I've made a few changes based on your feedback and some extra as well. Let me know what you think.

jsimnz

Added a few thoughts and changes. Most notably is the logger flow, route names/prefix, and the new context object.

jsimnz · 2022-04-27T22:32:06Z

api/http/handler.go

+	db client.DB
+
+	*chi.Mux
+	*logger


At the moment, the practise weve been using in the other packages, is a "package-level" logger with whatever appropriate namespace/name based on the package.

An example of that is here:

defradb/net/net.go

Line 20 in 27ffcf3

log = logging.MustNewLogger("defra.net")

I noticed that. I thought I would try it as a config see what the thought was. I'm happy to revert to package level logger.

jsimnz · 2022-04-27T22:33:45Z

api/http/handler.go

+
+// HandlerConfig holds the handler configurable parameters
+type HandlerConfig struct {
+	Logger logging.Logger


A continuation of my comments elsewhere, I dont think its necessary to pass in the logger to the handler package, and we certainly wouldnt need to define the loglevel/stacktrace/config info for the logger within this config obj, as the logger stuff is defined elsewhere (at the entry point, eg CLI package).

jsimnz · 2022-04-27T22:36:58Z

api/http/handler.go

+		db: db,
+	}
+
+	if c != nil {


I'll reiterate Fred's point that its common to have pointers as arguments (for several reasons) and then having appropriate nil checks within the funcs. Moreover, the compiler is able to make several optimizations if it knows there are specific nil checks within a func, and ensure that all future pointer access is optimized without the need for runtime nil checks.

jsimnz · 2022-04-27T22:45:51Z

api/http/handler.go

+	res http.ResponseWriter
+	req *http.Request
+	db  client.DB
+	log *logger


Same thing mentioned else, should use package level logger :)

I'll try something out with package level logger. I think it might be fun.

jsimnz · 2022-04-27T22:47:14Z

api/http/handler.go

+	return h
+}
+
+type context struct {


Although I do see that it also passes in the client.DB object.

One naming option is handlerContext

jsimnz · 2022-04-27T22:49:50Z

api/http/router.go

+	// setup logger middleware
+	h.Use(h.loggerMiddleware)
+
+	// define routes


We can prob namespace all these routes under /api/v1 or something for best practises :)

should probably then also update the information in all these docs and Makefile files in the respective lines to http://localhost:9181/api/v1/:

README.md 123:This will start a node with the default settings (running at http://localhost:9181), and create a configuration file at $HOME/.defra/config.yaml. Where $HOME is your operating system user home directory. 139:Once you've confirmed your node is running correctly, if you're using the GraphiQL client to interact with the database, then make sure you set the `GraphQL Endpoint` to `http://localhost:9181/api/v1/graphql`. docs/cmd/defradb_server-dump.md 21: --url string url of the target database (default "http://localhost:9181") docs/cmd/defradb_client_rpc.md 22: --url string url of the target database (default "http://localhost:9181") docs/cmd/defradb_client_rpc_addreplicator.md 27: --url string url of the target database (default "http://localhost:9181") docs/cmd/defradb_client_schema.md 24: --url string url of the target database (default "http://localhost:9181") docs/cmd/defradb_client_dump.md 20: --url string url of the target database (default "http://localhost:9181") docs/cmd/defradb_client_ping.md 20: --url string url of the target database (default "http://localhost:9181") docs/cmd/defradb_client_blocks_get.md 20: --url string url of the target database (default "http://localhost:9181") docs/cmd/defradb_client_query.md 31: --url string url of the target database (default "http://localhost:9181") docs/cmd/defradb_client_blocks.md 16: --url string url of the target database (default "http://localhost:9181") docs/cmd/defradb_client_schema_add.md 26: --url string url of the target database (default "http://localhost:9181") docs/cmd/defradb_start.md 30: --url string url of the target database (default "http://localhost:9181") docs/cmd/defradb.md 21: --url string url of the target database (default "http://localhost:9181") docs/cmd/defradb_client.md 22: --url string url of the target database (default "http://localhost:9181")

and just double checking that we don't need to change these ?:

cli/defradb/cmd/root.go 89: "http://localhost:9181", cli/defradb/cmd/config.go 118: Address: "localhost:9181",

discovering new parts of the code base every day 🤘

@shahzadlone I don't think we should update these URLs. The API endpoint and the database URL are two different things. So saying that the URL of the target database is http://localhost:9181 is correct in my opinion.

jsimnz · 2022-04-27T22:51:47Z

api/http/server_test.go

+	// test with no config
+	s := NewServer()
+	if ok := assert.NotNil(t, s); ok {
+		assert.Error(t, s.Listen(":303000"))


Is this just testing to make sure you cant listen on an invalid port/addr combo? If so, should rename the Test func.

At the moment it's pretty useless if you ask me as this is just a utility function using the standard library Listener. I did it mainly for code coverage and to have it as a place holder later on if that function ever becomes more complex and it becomes important to test.

jsimnz · 2022-04-27T22:54:48Z

api/http/logger.go

+	"github.com/sourcenetwork/defradb/logging"
+)
+
+type logger struct {


I don't think I see a point of having this type. As mentioned in other comments, the package should have a package-level logger that can be directly accessed.

If the logger is at the package level, 100% agree.

jsimnz · 2022-04-27T22:56:17Z

api/http/logger.go

+	}
+}
+
+type loggingResponseWriter struct {


This middleware handler can either accept logger as a parameter, or just call the package level logger, but doesn't need to use a custom logger type native to this package.

jsimnz · 2022-04-27T22:56:54Z

api/http/logger.go

+	return lrw.ResponseWriter.Write(b)
+}
+
+func (l *logger) loggerMiddleware(next http.Handler) http.Handler {


Reiteration of the logger design, this can be a regular func that can either take a logger.Logger or just call the package level logger directly.

having the package level logger has a lot of impact on the rest of the code. I'll do the changes :)

fredcarle · 2022-04-28T05:39:01Z

@AndrewSisley and @jsimnz, I made quite a few changes that cover your concerns and suggestions. Mainly the logger is now set at the package level, the context struct has been removed and no more passing around configs. I've also added some more http error handling. Overall the architecture is simpler than my original commit.

@shahzadlone I still haven't tested with the gql clients but I think my latest changes cover the issue that you were seeing.

shahzadlone · 2022-04-28T13:55:13Z

Title looks good!

I believe the new change of removing the GET specific if check should fix the GQL client issue I mentioned before.

However, I tested this branch at this commit b84e529 and there is something broken, it gives me the following error just loading in the schema.

Client Error:

2022-04-28T09:52:39.320-0400, INFO, defra.cli, cmd/schema_add.go:76, , {"Result": "404 page not found\n"}

Server Error:

2022-04-28T09:52:39.320-0400, INFO, defra.http, http/logger.go:58, Request, {"Method": "POST", "Path": "/schema/load", "Status": 404, "Length": 19, "Elapsed": 0.000014581}

shahzadlone · 2022-04-28T14:29:13Z

Found what the bug is.

You forgot to update the cobra commands under /cli/defradb/cmd/ for example in ./cli/defradb/cmd/schema_add.go

The db address is still:

			dbaddr = "http://" + dbaddr

Which needs to be updated to:

			dbaddr = "http://" + dbaddr + "/api/v1"

However maybe you can export the "/api/v1" const string from the api/http/router.go (but rename to something more specific rather than version).

api/http/router.go

AndrewSisley

LGTM :) Cheers for putting up with all our comments 😁

shahzadlone

LGTM ✅ Great work, WOOOT WOOOOT!!!

Tested these and can confirm they all work:

✅ Mutation Operation with GET on Insomnia
✅ Mutation Operation with GET on Altair
✅ Mutation Operation with POST on Insomnia
✅ Mutation Operation with POST on Altair
✅ Query Operation with GET on Insomnia
✅ Query Operation with GET on Altair
✅ Query Operation with POST on Insomnia
✅ Query Operation with POST on Altair

fredcarle · 2022-05-10T03:25:18Z

LGTM :) Cheers for putting up with all our comments 😁

No worries. One of the fastest way to improve team efficiency and personal proficiency is to have constructive exchanges. It's always constructive to get feedback:

Request to change but turns out it's better the way it already is --> Confirms knowledge
Request to change and turns out the proposed change is better --> Increase knowledge
Pushback on proposed change --> Break assumptions --> Increase knowledge

I love it. Specially when those interactions are with rock star devs 🤘

Co-authored-by: Shahzad Lone <[email protected]>

Rename test functions to be more specific. Remove api package and use http.NewServer. Put handlerFuncs in their own file. Change the way options are set on the handler.

Add better error handling. Remove the context struct. Use context.Context to pass db client. Remove handler options.

Move log to api.go Add more specific unit test Add ctx to handleErr Add suffix to handlerfuncs name Make Handler local Fix JoinPaths

Resolves sourcenetwork#372 Resolves sourcenetwork#367 This PR restructures the api/http package with idiomatic patterns and a few useful utility functions.

fredcarle added area/api Related to the external API component refactor This issue specific to or requires *notable* refactoring of existing codebases and components DO NOT MERGE labels Apr 27, 2022

fredcarle added this to the DefraDB v0.3 milestone Apr 27, 2022

shahzadlone requested changes Apr 27, 2022

View reviewed changes

shahzadlone assigned fredcarle Apr 27, 2022

fredcarle force-pushed the fredcarle/refactor/I372-http-api branch from eac7ea5 to dd46ebd Compare April 27, 2022 13:46

AndrewSisley requested changes Apr 27, 2022

View reviewed changes

fredcarle changed the title ~~refactor: HTTP API~~ refactor: Improve the HTTP API implementation Apr 27, 2022

jsimnz requested changes Apr 27, 2022

View reviewed changes

fredcarle force-pushed the fredcarle/refactor/I372-http-api branch from cf2ea6d to 05c9389 Compare April 28, 2022 04:04

shahzadlone added the action/no-benchmark Skips the action that runs the benchmark. label Apr 28, 2022

sourcenetwork deleted a comment from source-devs Apr 28, 2022

shahzadlone reviewed Apr 28, 2022

View reviewed changes

api/http/router.go Outdated Show resolved Hide resolved

fredcarle force-pushed the fredcarle/refactor/I372-http-api branch 2 times, most recently from 94e5570 to 67dc73e Compare April 29, 2022 00:05

fredcarle marked this pull request as ready for review April 29, 2022 12:15

fredcarle requested a review from AndrewSisley May 9, 2022 03:41

AndrewSisley approved these changes May 9, 2022

View reviewed changes

shahzadlone approved these changes May 10, 2022

View reviewed changes

fredcarle force-pushed the fredcarle/refactor/I372-http-api branch from c509a55 to 2365b9c Compare May 10, 2022 03:26

jsimnz approved these changes May 10, 2022

View reviewed changes

fredcarle force-pushed the fredcarle/refactor/I372-http-api branch from 2365b9c to 1264525 Compare May 11, 2022 22:17

fredcarle and others added 19 commits May 12, 2022 09:48

http api refactor draft

6a6da75

graphql post and setRoute fix

f7ad42f

update README and fix ioutil import

60edcc3

remove commented code

fa67697

Co-authored-by: Shahzad Lone <[email protected]>

fix missing route bug

722ae12

applying feedback

4e6600b

Rename test functions to be more specific. Remove api package and use http.NewServer. Put handlerFuncs in their own file. Change the way options are set on the handler.

logger as package level logger and more

38ca252

Add better error handling. Remove the context struct. Use context.Context to pass db client. Remove handler options.

fix unhandled errors, dead code and gql query

f2ffd8b

api version in path

9bb07d5

Add http api version to endpoint path

64aad49

Update chi version

871eebf

Add TestJoinPaths

6dcbf1d

Resolve review comments

5209812

Move log to api.go Add more specific unit test Add ctx to handleErr Add suffix to handlerfuncs name Make Handler local Fix JoinPaths

Add graphQL JSON body parsing

45a4533

Change to http package error strings

695a0f0

Add unit tests to get 100% coverage except handlerfuncs

fa44712

fix SendJSONWithWriteFailure

55c9bcf

add test for marshall error with following write error

f021c46

fix final suggestions

56862b5

fredcarle force-pushed the fredcarle/refactor/I372-http-api branch from 1264525 to 56862b5 Compare May 12, 2022 13:48

fredcarle merged commit 593214a into develop May 12, 2022

fredcarle deleted the fredcarle/refactor/I372-http-api branch May 12, 2022 19:54

refactor: Improve the HTTP API implementation #382

refactor: Improve the HTTP API implementation #382

Conversation

fredcarle commented Apr 27, 2022 • edited Loading

codecov bot commented Apr 27, 2022 • edited Loading

Codecov Report

shahzadlone left a comment • edited Loading

Choose a reason for hiding this comment

shahzadlone Apr 27, 2022 • edited Loading

Choose a reason for hiding this comment

AndrewSisley left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AndrewSisley Apr 27, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fredcarle commented Apr 27, 2022

fredcarle commented Apr 27, 2022

jsimnz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shahzadlone Apr 28, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fredcarle commented Apr 28, 2022

shahzadlone commented Apr 28, 2022

shahzadlone commented Apr 28, 2022 • edited Loading

AndrewSisley left a comment

Choose a reason for hiding this comment

shahzadlone left a comment • edited Loading

Choose a reason for hiding this comment

fredcarle commented Apr 27, 2022 •

edited

Loading

codecov bot commented Apr 27, 2022 •

edited

Loading

shahzadlone left a comment •

edited

Loading

shahzadlone Apr 27, 2022 •

edited

Loading

AndrewSisley Apr 27, 2022 •

edited

Loading

shahzadlone Apr 28, 2022 •

edited

Loading

shahzadlone commented Apr 28, 2022 •

edited

Loading

shahzadlone left a comment •

edited

Loading