feat(cms/api): add basic API client by Varun-Sethu · Pull Request #238 · csesoc/website

Varun-Sethu · 2022-09-20T04:37:04Z

PR adds a really small and basic API client for interfacing with the API, the client provides strict type definitions to make refactoring easier if we ever change the API contract between FE and BE (also serves as an implicit source of documentation)

…varun/APIClient

zax-xyz

doesn't lgtm 😎

zax-xyz · 2022-11-22T11:04:28Z

frontend/src/packages/api/filesystem.ts

+        return response.ok
+            ? (await response.json()).Response as ResponseType
+            : (await response.json()) as APIError;


what if response.json() fails? gotta double abstract error handling 😳

zax-xyz · 2022-11-22T11:06:28Z

frontend/src/packages/api/filesystem.ts

+import { CreateFilesystemEntryResponse, FilesystemEntry } from "./types/filesystem";
+import { APIError, EmptyAPIResponse } from "./types/general";


thoughts on using import type since none of these are being used as values?

related: https://typescript-eslint.io/rules/consistent-type-imports/

oooh I didnt even know this was a thing 😎

zax-xyz · 2022-11-22T11:12:08Z

frontend/src/packages/api/filesystem.ts

+// Only interface with the BE FS APIs via this class
+export class FilesystemAPI {
+    // GetEntityInfo retrieves all entity information for an FS entity given its ID
+    public static GetEntityInfo = (EntityID: string): Promise<FilesystemEntry | APIError> => 


thoughts on abstracting these return types since they're all of the same format (Promise<T | APIError>)?

IMO I feel like that's kinda unnecessary, its a single line type

zax-xyz · 2022-11-22T11:14:06Z

frontend/src/packages/api/filesystem.ts

+
+    // SendGetRequest is a small helper functions for sending get request and wrapping the response in an appropriate type
+    static async SendGetRequest<ResponseType> (url: string): Promise<ResponseType | APIError> {
+        const response = await fetch(`${API_URL}${url}`);


thoughts on catching errors here as well (e.g., invalid url or dns lookup failures)? (ayo triple abstraction??)

zax-xyz · 2022-11-22T11:17:58Z

frontend/src/packages/api/types/filesystem.ts

+export const hasFieldOfType = (o: any, fieldName: string, type: string): boolean =>
+    fieldName in o && (typeof o[fieldName]) === type;


not sure if it makes more sense to use fieldType here since you're using fieldName as well - i got really confused here for a minute because i thought type was some sort of built-in, or some typescript magic was going on here 😳

yeah thats a good point, shall rename to fieldType

zax-xyz · 2022-11-22T11:19:25Z

frontend/src/packages/api/types/filesystem.ts

+    hasFieldOfType(o, "EntityName", "string") &&
+    hasFieldOfType(o, "IsDocument", "boolean") &&
+    hasFieldOfType(o, "Parent", "string") && 
+    hasFieldOfType(o, "Children", typeof []) &&


this isn't very reliable since typeof [] is just "object" (javascript moment) - you probably want to use Array.isArray here

zax-xyz

thoughts on using stricter linter configurations and/or an autoformatter?

frontend/src/packages/api/tests/filesystemConsistency.test.ts

frontend/src/packages/api/filesystem.ts

frontend/src/packages/api/tests/filesystemConsistency.test.ts

frontend/src/packages/api/filesystem.ts

frontend/src/packages/api/types/filesystem.ts

frontend/src/packages/api/types/general.ts

lhjt · 2022-11-22T14:13:58Z

.github/workflows/ci.yml

+        run: |
+          GO_MOD=go.mod docker compose --env-file=./config/ci.env.dev up --wait --build backend db staging_db frontend
+      - name: Backend Logs
+        run: |
+          docker logs go_backend
+          docker logs pg_container
+          docker ps
+          docker logs go_backend
+#         docker exec frontend curl http://backend:8080/api/filesystem/info
+# docker exec frontend npm test


Uh sorry so just to clarify but how is the docker compose build part running any sort of tests? And should should the lines at the bottom be commented out?

cc @sachk

smh i pointed this out and now jared is STEALING the credit - more ABUSE by csesoc execs :(

The bottom two lines were commented for testing reasons (they should not be commented out), the entire PR is still blocked by the weird networking issue 😢

Co-authored-by: Michael Vo <zax@zaxu.xyz>

Varun-Sethu · 2022-11-22T22:23:01Z

thoughts on using stricter linter configurations and/or an autoformatter?

Hmm seems like its worth doing although u probably want to talk to the FE team abt that

hdphuong · 2022-11-23T08:38:59Z

frontend/src/packages/api/tests/filesystemConsistency.test.ts

+
+        // publish it
+        const publishResp = await FilesystemAPI.RenameEntity(newEntityId, "docMcStuffins");
+        expect(IsEmptyApiResponse(publishResp), 'Expected deletion response to be assignable to empty');


tsk tsk 👆👆

hdphuong · 2022-11-23T08:40:57Z

frontend/src/packages/api/tests/filesystemConsistency.test.ts

+
+        // Create a document
+        const newDocument = await FilesystemAPI.CreateDocument("ebic document of truth", root.EntityID);
+        expect(IsCreateFilesystemEntryResponse(newDocument), "Expected CreateDocument response to be assignable to CreateFilesystemEntryResponse").toBe(true);


shouldn’t createDocument take in 3 arguments?

hdphuong · 2022-11-23T08:42:02Z

frontend/src/packages/api/filesystem.ts

+        FilesystemAPI.SendGetRequest<FilesystemEntry>(`/api/filesystem/info`);
+
+    // CreateEntity constructs an editable un-published filesystem entry
+    public static CreateDocument = (Name: string, ParentID: string, ownerGroup = 1): Promise<CreateFilesystemEntryResponse | APIError> => 


nitpicking but it should be CreateDocument not CreateEntity in the comment

Also, some filesystem endpoints are missing, is that intentional 👀

hdphuong · 2022-11-23T08:43:47Z

backend/main.go

 	handler = c.Handler(handler)

+	log.Print("CMS Go backend starting on port :8080 :D.")
+	log.Print("Amongus.")


Varun-Sethu added 19 commits September 20, 2022 14:35

Adding basic API client

736dd1f

added API type assertations

d8bb2cf

Merge branch 'main' of github.com:csesoc/cms.csesoc.unsw.edu.au into …

f64cae6

…varun/APIClient

fixing expect message

4a3207a

updating workflow to support integration tests

a9394cf

undoing changes to ghost exporter

7567dc2

undoing changes to ghost exporter

efcf807

oh yaml gods please work

ae353a3

i smite thee yaml gods

72652d8

all i know is pain and suffering

720a94f

please jsut run the stupid backend tests

95794e3

i think this works now

167b3fc

tests pass (locally)

db91d04

i guess not

9045648

:(

dfe482d

please just just work please pleaaase

4c7f9b6

i dont know how to title these commits anymore

b44edd2

i dont know how to title these commits anymore

f6c0ba4

upgrading ubuntu version

2da112b

Varun-Sethu force-pushed the varun/APIClient branch from b03b0e8 to 2da112b Compare September 21, 2022 12:41

Varun-Sethu added 10 commits September 21, 2022 22:43

upgrading ubuntu version

2e0c432

i think this might be it! :D

18066b3

adding CI configuration

e1315d5

fixing client config

e82e62b

throws hands in air

1abd37a

i reaaallly hope this works

92a6c76

hopefully this works again

8374a04

hopefully this works again

dc36f41

printf debugging begins

aede6b6

capturing logs

1f5189d

backend logging

75b8261

Varun-Sethu force-pushed the varun/APIClient branch 7 times, most recently from ad8b3b8 to 91d59fd Compare September 22, 2022 03:59

backend logging

e34370d

Varun-Sethu force-pushed the varun/APIClient branch from 91d59fd to e34370d Compare September 22, 2022 04:00

lhjt changed the title ~~Adding basic API Client~~ feat(backend/api): add basic API Client Sep 23, 2022

lhjt changed the title ~~feat(backend/api): add basic API Client~~ feat(cms/api): add basic API Client Sep 23, 2022

lhjt changed the title ~~feat(cms/api): add basic API Client~~ feat(cms/api): add basic API client Sep 23, 2022

lhjt added enhancement New feature or request cms labels Sep 23, 2022

lhjt requested a review from a team as a code owner September 23, 2022 12:43

lhjt requested a review from a team as a code owner September 23, 2022 13:51

lhjt requested a review from fafnirZ as a code owner September 28, 2022 13:27

zax-xyz suggested changes Nov 22, 2022

View reviewed changes

lhjt requested changes Nov 22, 2022

View reviewed changes

Varun-Sethu requested review from hdphuong, jamest0 and lauraw0 as code owners November 22, 2022 22:19

Varun-Sethu and others added 4 commits November 23, 2022 09:19

Update frontend/src/packages/api/tests/filesystemConsistency.test.ts

5ec1dec

Co-authored-by: Michael Vo <zax@zaxu.xyz>

Update frontend/src/packages/api/types/general.ts

4abce87

Co-authored-by: Michael Vo <zax@zaxu.xyz>

Apply suggestions from code review

41c6103

Co-authored-by: Michael Vo <zax@zaxu.xyz>

Apply suggestions from code review

1bc173a

Co-authored-by: Michael Vo <zax@zaxu.xyz>

hdphuong reviewed Nov 23, 2022

View reviewed changes

		import { CreateFilesystemEntryResponse, FilesystemEntry } from "./types/filesystem";
		import { APIError, EmptyAPIResponse } from "./types/general";

		export const hasFieldOfType = (o: any, fieldName: string, type: string): boolean =>
		fieldName in o && (typeof o[fieldName]) === type;

Conversation

Varun-Sethu commented Sep 20, 2022

Uh oh!

zax-xyz left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zax-xyz Nov 22, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zax-xyz Nov 22, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zax-xyz left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Varun-Sethu Nov 22, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Varun-Sethu commented Nov 22, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

zax-xyz Nov 22, 2022 •

edited

Loading

zax-xyz Nov 22, 2022 •

edited

Loading

zax-xyz left a comment •

edited

Loading

Varun-Sethu Nov 22, 2022 •

edited

Loading