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

feat: Lookup requests for BB API #293

Merged
merged 67 commits into from Aug 1, 2019

Conversation

@akhilesh26
Copy link
Contributor

commented Jun 17, 2019

BB API

Run this initial setup for BB API.
Install dependencies using npm install
To check the endpoint, run the project without docker with npm run start-api.
Endpoints implemented:
http://localhost:9098/<entity>/<bbid>
http://localhost:9098/<entity>/<bbid>/aliases
http://localhost:9098/<entity>/<bbid>/identifiers

To run the tests of api use npm test ./test/src/api

@coveralls

This comment has been minimized.

Copy link

commented Jun 17, 2019

Coverage Status

Coverage increased (+2.5%) to 42.919% when pulling 95edee5 on akhilesh26:api2 into 98f6484 on bookbrainz:master.

src/api/helpers/formatEntityData.js Outdated Show resolved Hide resolved
src/api/helpers/formatEntityData.js Outdated Show resolved Hide resolved
@MonkeyDo

This comment has been minimized.

Copy link
Contributor

commented Jun 20, 2019

For the relationships lookup endpoint:

{
    "bbid": "ba446064-90a5-447b-abe5-139be547da2e",
    "relationships": [
        {
            "id": 1626,
            "typeId": 8,
            "sourceBbid": "6d5c4a15-3d08-41bc-9bbf-c5fd2dbebf52",
            "targetBbid": "ba446064-90a5-447b-abe5-139be547da2e",
            "type": {
                "id": 8,
                "label": "Author",
                "description": "This relationship is used to link a work to its author.",
                "linkPhrase": "wrote",
                "sourceEntityType": "Author",
                "targetEntityType": "Work",
                "parentId": null,
                "childOrder": 1,
                "deprecated": false,
                "reverseLinkPhrase": "was written by"
            },
            "_pivot_set_id": 2847,
            "_pivot_relationship_id": 1626
        }
    ]
}
  • Note that in the response JSON, target and targetEntityType do not mean the same thing as in the original relationship.
    In the response, the target is always the other entity (not the one we are querying)
  1. Determine if queried entity is source or target of the original relationship (compare bbid to relationships..sourceBbid or targetBbid)
    In this case, bbid = targetBbid, so the current entity is the target of the relationship (Work written by Author, the Work is the target)
  2. From that information, determine the 'direction' and the 'linkPhrase' (using linkPhrase or reverseLinkPhrase depending on direction)
    In this case, direction = "backward" , so we use reverseLinkPhrase
  3. Rearrange the data and create your response JSON. We try to send back only the strict necessary.
    The rel description for example will be found in the documentation, no need to send strings around and use bandwidth.
    I am making an exception for relationshipType.name to make the relationship human-readable. We use the relationship.type.label which is usually a short string

The response:

{
	"bbid": "ba446064-90a5-447b-abe5-139be547da2e",
	"relationships": [
		{
			"id": 1626,
			"targetBbid": "6d5c4a15-3d08-41bc-9bbf-c5fd2dbebf52",
			"targetEntityType": "Author"
			"direction": "backward"
			"linkPhrase": "was written by",
			"relationshipType": {
				"name" : "Author",
				"id": 8
			},
		}
	]
}

From that info, I can reconstitute the relationship:
Entity type I'm querrying - bbid - linkPhrase - targetEntityType - targetBbid
The "Work" - "ba446064-90a5-447b-abe5-139be547da2e" - "was written by" - "Author" - "6d5c4a15-3d08-41bc-9bbf-c5fd2dbebf52"

If I queried the Author relationships, I would find:

{
	"bbid": "6d5c4a15-3d08-41bc-9bbf-c5fd2dbebf52",
	"relationships": [
		{
			"id": 1626,
			"targetBbid": "ba446064-90a5-447b-abe5-139be547da2e",
			"targetEntityType": "Work"
			"direction": "forward"
			"linkPhrase": "wrote",
			"relationshipType": {
				"name" : "Author",
				"id": 8
			},
		}
	]
}

"Author" - "6d5c4a15-3d08-41bc-9bbf-c5fd2dbebf52" - "wrote" - "Work" - "ba446064-90a5-447b-abe5-139be547da2e"

@MonkeyDo
Copy link
Contributor

left a comment

Relationships are looking goo so far !
Minor changes below.

I also wonder if

"relationshipType": {
    "name": "Author",
    "id": 8
}

would be better expressed as

"relationshipTypeName":  "Author",
"relationshipTypeId": 8

What do you think?

src/api/helpers/formatEntityData.js Outdated Show resolved Hide resolved
src/api/helpers/formatEntityData.js Outdated Show resolved Hide resolved
src/api/helpers/formatEntityData.js Outdated Show resolved Hide resolved
@akhilesh26

This comment has been minimized.

Copy link
Contributor Author

commented Jun 21, 2019

Hello @MonkeyDo,
Now all lookup endpoints except authors-credits are implemented with tests. Please give a detailed review when you have time. then now I will start documenting the code as well as endpoint API using JSdoc.

Thanks!

@MonkeyDo
Copy link
Contributor

left a comment

Everything is working nicely!

@akhilesh26

This comment has been minimized.

Copy link
Contributor Author

commented Jun 28, 2019

Hello @MonkeyDo ,
I am trying to add relationship but it through some error, please help to add one and more relationship in create-entity.js so that I could write some more tests.

error:

1) GET /Author
       "before all" hook:
     insert into "bookbrainz"."relationship" ("id", "source_bbid", "target_bbid", "type_id") values ($1, $2, $3, $4) returning "id" - insert or update on table "relationship" violates foreign key constraint "relationship_source_bbid_fkey"
  error: insert or update on table "relationship" violates foreign key constraint "relationship_source_bbid_fkey"

`

@MonkeyDo

This comment has been minimized.

Copy link
Contributor

commented Jun 30, 2019

Hello @MonkeyDo ,
I am trying to add relationship but it through some error, please help to add one and more relationship in create-entity.js so that I could write some more tests.

error:

1) GET /Author
       "before all" hook:
     insert into "bookbrainz"."relationship" ("id", "source_bbid", "target_bbid", "type_id") values ($1, $2, $3, $4) returning "id" - insert or update on table "relationship" violates foreign key constraint "relationship_source_bbid_fkey"
  error: insert or update on table "relationship" violates foreign key constraint "relationship_source_bbid_fkey"

`

That usually means you need to first insert another element that the relationship schema depends on.
In this case, foreign key constraint "relationship_source_bbid_fkey" tells use that the source bbid does not point to an existing entity.
The relevant line in the schema is: ALTER TABLE bookbrainz.relationship ADD FOREIGN KEY (source_bbid) REFERENCES bookbrainz.entity (bbid);

So in create-entity, where you insert a relationship, you need to make sure the bbids refer to existing entity. So in createRelationshipSet you'll want to first call the methods to create entities (createWork, createAuthor, etc) depending on the relationship source and target types you want.

@MonkeyDo
Copy link
Contributor

left a comment

Glad to see the documentation shaping up!
Can't wait to see the swagger integration and the documentation and examples of the API endpoints!

src/api/helpers/formatEntityData.js Show resolved Hide resolved
src/api/helpers/formatEntityData.js Outdated Show resolved Hide resolved
src/api/helpers/formatEntityData.js Outdated Show resolved Hide resolved
src/api/helpers/entityLoader.js Outdated Show resolved Hide resolved
src/api/helpers/utils.js Outdated Show resolved Hide resolved
src/api/helpers/entityLoader.js Outdated Show resolved Hide resolved
src/api/helpers/formatEntityData.js Outdated Show resolved Hide resolved
@akhilesh26

This comment has been minimized.

Copy link
Contributor Author

commented Jul 3, 2019

@MonkeyDo thanks for detailed review. I will refactor all ASAP.

@MonkeyDo

This comment has been minimized.

Copy link
Contributor

commented Jul 4, 2019

@akhilesh For documenting the API routes, I suggest using a combination of jsdoc, swagger (OpenAPI specification) and expressJS.
The idea is to auto-generate a documentation webpage from jsdoc comments.

This article explains how to set it up: http://www.acuriousanimal.com/2018/10/20/express-swagger-doc.html

You'll also want to read more about the swagger-jsdoc package for the syntax to use (a little bit different from regular jsdoc) : https://github.com/Surnet/swagger-jsdoc/blob/master/docs/GETTING-STARTED.md

@akhilesh26

This comment has been minimized.

Copy link
Contributor Author

commented Jul 5, 2019

Hello @MonkeyDo ,
I tried to integrate swagger for API documentation, but find difficult to do that successfully. When I run the API and goto http://localhost:9098/api-docs/ , it shows only swagger page, a documentation comment written at routed/auther.js is not showing. Means, SwaggerUI not showing endpoints.
Please help to fix this swagger integration.

@akhilesh26

This comment has been minimized.

Copy link
Contributor Author

commented Jul 9, 2019

Hello @MonkeyDo,
I wrote the swagger documentation of some basic route. Please check these. First, we will fix swagger-jsdoc issue in our project then I will update the jsdoc in the route file. I wrote YAML in online swagger editor to check. Here is the link: https://app.swaggerhub.com/apis-docs/akhilesh26/BookBrainz/1.0.0#/
Please give some time to me so that we can resolve these issues, then merge this pull request and live API lookup.

@MonkeyDo MonkeyDo changed the title feat: Lookup requests for BB API uljfeat: Lookup requests for BB API Jul 23, 2019

@MonkeyDo MonkeyDo changed the title uljfeat: Lookup requests for BB API feat: Lookup requests for BB API Jul 23, 2019

@MonkeyDo

This comment has been minimized.

Copy link
Contributor

commented Jul 23, 2019

Hello @MonkeyDo ,
I tried to integrate swagger for API documentation, but find difficult to do that successfully. When I run the API and goto http://localhost:9098/api-docs/ , it shows only swagger page, a documentation comment written at routed/auther.js is not showing. Means, SwaggerUI not showing endpoints.
Please help to fix this swagger integration.

This was a very easy fix that only took me two minutes. I wish you were a bit more independant and try to resolve the issues you encounter yourself.
Trying to fix issues by reading documentation is the best teaching I can think of, and a skill you must acquire. I know it's often hard to know where to start, or what could be going wrong, but by methodically isolating the issue and reading the documentation you'll be able to resolve 99% of problems.
This is absolutely critical if you want to be a professional developer. You won't always have a mentor (or they could be unreachable, as the past two weeks have proved), and you need to be able to solve your own issues.

In this case, you would have found an answer to your problem in the first section of the swagger-jsdoc "getting started" guide:
relative paths in apis are relative to the current directory from which the Node.js program is ran, not the application serving the APIs.
In your swagger.js file, changing apis: ['./routes/author.js'] to apis: ['src/api/routes/author.js'] solves your issue, and the route show up on the local api-docs swagger page.

[EDIT]: sorry, I just saw the other docs on swaggerhub, disregard this comment
I would also have expected you to continue documenting the other endpoints, even if the presentation wasn't working. Documentation is always useful, even if it is only comments in the file itself.

@akhilesh26

This comment has been minimized.

Copy link
Contributor Author

commented Jul 24, 2019

Thanks, @MonkeyDo for resolving the issue.
I am very sorry for this little issue of swagger. I was tried but not successful. I will try to avoid this kind of mistakes. thanks for encouraging.

@akhilesh26 akhilesh26 closed this Jul 24, 2019

@akhilesh26 akhilesh26 reopened this Jul 24, 2019

@akhilesh26

This comment has been minimized.

Copy link
Contributor Author

commented Jul 26, 2019

Hello @MonkeyDo,
Please check the last commit and notify the changes then I will complete swagger-jsdoc for all lookup endpoints accordingly. Check the description and other English mistakes, please.

@MonkeyDo
Copy link
Contributor

left a comment

A few corrections, but not much. Overall this is good work, clean and clear.
You can go ahead and document the other endpoints in the same way.

src/api/routes/author.js Outdated Show resolved Hide resolved
src/api/routes/author.js Outdated Show resolved Hide resolved
src/api/routes/author.js Outdated Show resolved Hide resolved
src/api/routes/author.js Outdated Show resolved Hide resolved
src/api/routes/author.js Outdated Show resolved Hide resolved
src/api/routes/author.js Outdated Show resolved Hide resolved
src/api/routes/author.js Outdated Show resolved Hide resolved

@MonkeyDo MonkeyDo force-pushed the akhilesh26:api2 branch from 7633f5c to f5deead Jul 31, 2019

@akhilesh26

This comment has been minimized.

Copy link
Contributor Author

commented Jul 31, 2019

Hi @MonkeyDo,
Now, this PR is showing 119 commits to merge, What did happen with this? Have I done any my mistake in resolving conflicts? Or Is it because of rebasing? Will it create any problem to master?

@MonkeyDo

This comment has been minimized.

Copy link
Contributor

commented Aug 1, 2019

Hi @MonkeyDo,
Now, this PR is showing 119 commits to merge, What did happen with this? Have I done any my mistake in resolving conflicts? Or Is it because of rebasing? Will it create any problem to master?

Yes, I'm afraid you did a merge instead of a rebase, or if you did a rebase, you pulled/pushed instead of force-pushing.
The good news is it doesn't look like it matters, apart from the messy git history.

You have to remember the process (it took me a long time to get comfortable with it…):

  1. Ensure you don't have any unstaged changes on your local branch (and ensure you're on the right branch !).
  2. git rebase master
  3. Fix your conflicts, then `git rebase --continue``
  4. Once the rebase is finished, do not pull !
  5. git push --force-with-lease to force-push your rebased branch
  6. You can always to git rebase --abort during the process and start over if you get confused. It sure helps me to know I can abort and get back to where I was before starting.

What happened, I believe is that at step 4/5 you did git pull then git push (instead of doing a force-push), which results in duplicated commit history.
Not the end of the world, but you should definitely get comfortable with the rebase process at it is very useful (especially for long PRs !).

If you're using GitHub Desktop instead of the terminal, make sure you're using the latest version. They recently added a "Force push" button (instead of the usual pull&push) when you finish a rebase.
When in doubt, I would always recommend checking the command-line. GitHub Desktop is nice to inspect changes and commits, but I always feel more in control with the command line for a rebase. It is well worth getting comfortable with.

@MonkeyDo MonkeyDo force-pushed the akhilesh26:api2 branch from 42a3ac7 to 7117397 Aug 1, 2019

@MonkeyDo

This comment has been minimized.

Copy link
Contributor

commented Aug 1, 2019

@akhilesh26 I was able to clean up the commit history, after a bit of thinking and trying things.

I always welcome an occasion to learn more and test my git skills.

Here's what I did:

  • git log --graph --decorate --pretty=oneline --abbrev-commit gives you a nice-to-look-at and clean git commit history.
  • I took note of the commits of interest. In this case:
    • the last commit before your merge (my commit b37ed56)
    • the merge commit itself (2b38f42)
    • the commit after the merge, the new aliases/… structure tests which we want to keep (42a3ac7).
  • I then did git reset --hard b37ed56 to go back in time to before the merge
  • then git cherry-pick 42a3ac7 to put your tests commit on top of that
  • git push --force-with-lease github-desktop-akhilesh26 HEAD:api2 to force-push the result to this PR
  • notice that I don't do anything with the merge commit. In this case, I wanted to entirely ignore it in the commit history.

Rewriting history on a branch that is public is usually a no-no. However, in this case since it is only the two of us working here, I can tell you that I have done a history rewrite, and you may have to delete your local branch and check it out again.

@MonkeyDo
Copy link
Contributor

left a comment

After a final review, I believe this is ready to merge ! 💥 🎉

@MonkeyDo MonkeyDo merged commit 07fd0f6 into bookbrainz:master Aug 1, 2019

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage increased (+2.5%) to 42.919%
Details
@MonkeyDo

This comment has been minimized.

Copy link
Contributor

commented Aug 1, 2019

Well done @akhilesh26 !
I'm really happy with how this turned out so far. Good quality code and tests and docs !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.