Skip to content

Conversation

@anthonykhoa
Copy link
Collaborator

@anthonykhoa anthonykhoa commented Oct 2, 2020

Functions to start and stop a connection to an Arango database, as well as delete and create users from an Arango database, need to be created. These functions are needed before creating the API routes that will call those functions.

This PR will:

  • Create a sendFetch helper function to use in the back end.
  • In the database/elastic/elastic.js file, sendESRequestis instead replaced with the sendFetch function.
  • Create functions to start and stop a connection to a Arango Database
  • Create functions to delete and create users on a Arango Database
  • Delete all neo4j files

@codecov
Copy link

codecov bot commented Oct 2, 2020

Codecov Report

Merging #233 into master will not change coverage.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff            @@
##            master      #233   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           12        13    +1     
  Lines          445       481   +36     
  Branches        43        50    +7     
=========================================
+ Hits           445       481   +36     
Impacted Files Coverage Δ
database/arango/arango.js 100.00% <100.00%> (ø)
database/elasticsearch/elastic.js 100.00% <100.00%> (ø)
lib/sendFetch.js 100.00% <100.00%> (ø)
src/server.js 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 642b583...fe193b8. Read the comment docs.

@anthonykhoa anthonykhoa changed the title closes #232 - Arango functions to stop/start server, and create/delete account [DNM] closes #232 - Arango functions to stop/start server, and create/delete account Oct 2, 2020
@anthonykhoa anthonykhoa marked this pull request as ready for review October 2, 2020 01:32
// returns array of objects containing a '_name' key.
// Ex [{ _name: 'lol' }, { _name: 'cakes' }]
const names = await db.databases();
return names.map((db) => db._name).includes(name);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems that this function requests all of the data from the database in JSON form and then search for the name. I think this can cause some inefficiency. Just like postgres and elasticsearch, I assume that there is a way to query the databases with specific name using arango.

Copy link
Collaborator Author

@anthonykhoa anthonykhoa Oct 3, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah yes there is, I considered doing it that way, but eventually concluded that it is better to stick to using their arangojs API in order to keep the code more consistent.

Another way I could do this is by making a request to a http REST api that arango provides. This is how deleting an account is done -- I use their http REST API. Here are the pros and cons of using each method to check if a user exists:

Using http REST api:

  • Pros
    1. Queries for only one user, so when the promise is received in const names = await db.databases(), I don't have to use map and includes functions.
  • Cons
    1. Have to make 2 requests to an arango database -- the first time to get a jwt token, the second time to check if a user exists. This is a type of additional logic that arangojs api would not need.

Using arangojs api:

  • Pros
    1. Only 1 request is made to an arango database -- getting a jwt is not necessary because we authenticated ourselves in the function to connect to the arango database.
    2. Keeps the code consistent. The functions to stop and start a connection to an arango database, as well as the methods to create a new user, create a database, and delete a database, are done using their arangojs api. Therefore using arangojs api to check if a user exists is appropriate because it keeps the code more consistent.
  • Cons
    1. Queries for more than one user, so additional logic must be performed to check if a user exists, after receiving the promise (using .map and .includes). Using http REST api, I would not need this type of additional logic.

Efficiency

I believe there is no noticeable difference in efficiency between the two methods, unless learndatabases.dev gets a looot of users. You can see here that on average, it took arango only 1.07 seconds to aggregate a collection of 1,632,803 documents. Since efficiency is not a concern, I think using arangojs is better for now. In the future, if efficiency becomes a concern, then we can switch to using the http REST api that arango provides.

Complexity

I think both methods are equally complex. With http REST api you have to first send a request to get a jwt token, and then use it in the next request to check if a user exists. With arangojs api, you have to filter through the array you receive after using db.databases().

Copy link
Collaborator

@hoiekim hoiekim Oct 5, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As you said it won't be a big problem for now since we don't have a lot of requests but I want our app to be open to a chance to be so.

I have done some research and found something that can be a clue.

  1. https://arangodb.github.io/arangojs/latest/classes/_database_.database.html#query
    This function let us use AQL
  2. https://www.arangodb.com/docs/stable/aql/tutorial-filter.html
    This is how to get document that matches specific condition

Copy link
Collaborator Author

@anthonykhoa anthonykhoa Oct 5, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have already tried looking into using AQL commands to do user management, but I wasn't able to do so. This is why I used http rest API to delete arango users instead -- because I couldn't use AQL commands to do so. It is possible for us to make a collection of users to query from, but that would make our code more complex than it has to be, and so we should not do that.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@songz songz merged commit 624355a into garageScript:master Oct 6, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants