gRPC implementation #2
Conversation
There was a problem hiding this comment.
Thank you for all this work. I'm looking forward to finally use a completely working mock backend, especially as we can finally implement e2e tests now.
Still there are some things (especially code style and documentation) that are not at level I want to accept without changes or discussion.
However, it seems to work perfectly, at least as far as I tested it for now.
A general advice: please make the PRs a little bit smaller.
.gitmodules
Outdated
| [submodule "api"] | ||
| path = api | ||
| url = [email protected]:SE-UUlm/snowballr-api.git | ||
| branch = feat/initial-api |
There was a problem hiding this comment.
I think you forgot to change this branch. feat/initiali-api doesn't exist anymore and we should also use main and no feature branch.
There was a problem hiding this comment.
I did miss this, yes.
| RUN npm install | ||
| RUN npm run compile:proto | ||
| RUN npm run build | ||
| CMD ["npm", "run", "start"] |
There was a problem hiding this comment.
Only a question of interest: why do you use this format to run a command not RUN npm run start?
There was a problem hiding this comment.
RUN npm run start would execute that command during build time. The server should be started when running the docker container, thus CMD is used instead of RUN. If you however meant why I put the command in brackets, I don't know. It's valid either way.
| @@ -0,0 +1,47 @@ | |||
| # SnowballR Mock Backend | |||
|
|
|||
There was a problem hiding this comment.
Great that you added a README.md file, I like it a lot! However, imo it would be even better to have one or two introductory sentences explaining what the mock backend is, what it is not and why we need it. Because we know its purpose etc., but somebody else not.
There was a problem hiding this comment.
Added a few introductory sentences. 👍
|
|
||
| ```sh | ||
| npm run start | ||
| ``` |
There was a problem hiding this comment.
So do i understand correctly, that this would not work directly, but I have to start the envoy proxy separately. If so, please mention this.
There was a problem hiding this comment.
Changed out the proxy and thus updated the README. Should be clearer now.
README.md
Outdated
| ## Tooling | ||
|
|
||
| Both [grpcurl](https://github.com/fullstorydev/grpcurl) and | ||
| [grpcui](https://github.com/fullstorydev/grpcui) can be very helpful when | ||
| getting to know the api or debugging. Install the ones you like and use them | ||
| like this (replacing the address if needed): | ||
|
|
||
| ```sh | ||
| grpcui -plaintext 127.0.0.1:8080 | ||
| grpcurl -plaintext 127.0.0.1:8080 snowballr.SnowballR.IsAuthenticated | ||
| ``` | ||
|
|
There was a problem hiding this comment.
Very useful section, thank you!
But the default ist 8081, isn't it? (at least it was on my machine)
There was a problem hiding this comment.
One minor request: I think it would be nice to add a hint, that you to register first, because no default user exist for now and use this access token to add a Authorization field in the request. Because this was not totally clear to me at the beginning.
src/service.ts
Outdated
| ): void { | ||
| const { id } = call.request; | ||
| if (project_papers.has(id)) { | ||
| callback(null, project_papers.get(id)!); |
There was a problem hiding this comment.
I do not really understand what this method is supposed to do? What is this id, a paper id? And then a get a paper from any project, even if I am not part of? If so, what's the difference to the getPaperById method?
There was a problem hiding this comment.
It is the id of the association of a paper with a project.
src/service.ts
Outdated
| ): void { | ||
| const { projectId, paperId, stage } = call.request; | ||
| if (project_papers.has(projectId)) { | ||
| const id = project_papers.size.toString(); |
There was a problem hiding this comment.
I don't think this Id is unique this way.
There was a problem hiding this comment.
You're right. Fixed it.
src/service.ts
Outdated
| selectedCriteriaIds: selectedCriteriaIds, | ||
| }); | ||
|
|
||
| paper_reviews.get(projectPaperId)!.push(id); |
There was a problem hiding this comment.
I thought the reviews are stored in the project paper?
There was a problem hiding this comment.
They are associated with them using an extra map.
src/service.ts
Outdated
| for (const [projectPaperId, reviews] of paper_reviews) { | ||
| paper_reviews.set( | ||
| projectPaperId, | ||
| reviews.filter((c) => c != id), | ||
| ); |
There was a problem hiding this comment.
Maybe it is too late, but I don't understand this delete method. Why don't you delete the review from the reviews list and from the paper_reviews? And again, I thought the IDs are also stored in the project paper?
There was a problem hiding this comment.
There is no need to delete the review. Helps when determining a unique id.
src/service.ts
Outdated
| getForwardReferencedPapers: function ( | ||
| _: ServerUnaryCall<Id, Paper_List>, | ||
| callback: sendUnaryData<Paper_List>, | ||
| ): void { | ||
| callback(null, { | ||
| papers: Array.from(papers.values()), | ||
| }); | ||
| }, | ||
| getBackwardReferencedPapers: function ( | ||
| _: ServerUnaryCall<Id, Paper_List>, | ||
| callback: sendUnaryData<Paper_List>, | ||
| ): void { | ||
| callback(null, { | ||
| papers: Array.from(papers.values()), | ||
| }); | ||
| }, |
There was a problem hiding this comment.
I just reply with arbitrary papers. Just to get something displayed.
|
Hi, not sure if this was already mentioned. The README should contain the information that there's a submodule which needs to be set up. |
|
Also, there should be a |
Actually that is mentioned in the snowballr-api repo or do you mean how to initialize a git submodule? |
In my opinion, we should provide a step-by-step instruction of how a new developer can start the mock server without having to figure it out themselves. I.e.:
This should all go in a "How to get started" section where no questions are left open. |
This allows incorporating it directly into the main server instead of having to rely on envoy and docker compose
This pr implements the gRPC api specified in snowballr-api. Some functions are implemented as dummies. Authentication is handled using plain-text, be warned!