berryjen / Travel-API

1 stars 1 forks source link

API should respond with a list of countries #2

Open billglover opened 2 years ago

billglover commented 2 years ago

Our API currently responds with "Hello World". We need it to respond with a list of countries in JSON format. We use JSON as a way to return structured data that can be consumed (or used) by clients.

One example could be:

{
  "countries": [
    {
      "name": "country 1",
      "visited": true
    },
    {
      "name": "country 2",
      "visited": false
    },
    {
      "name": "country 3",
      "visited": true
    }
  ]
}

Note: The list of countries does not need to be real data. It can be hard-coded. We also don't need to worry about how data is sorted.

To be successful we expect the following:

berryjen commented 2 years ago

We have learned the concept of path & path parameters and extrapolated our knowledge to further build upon the API.

We have created a path "/uk" to which now it responds with an object, with the country name (England so far), with a visited property returning a Boolean value of T/F.

app.get('/uk', (req, res) => {
   res.json({
     name: "England",
     visited: true
   });
});

The next step is to create a "/" path that will return an array of visited and non-visited countries, as objects, say list of 10 to begin with.

berryjen commented 2 years ago

After my attempt of creating the 3 different paths: "/", "/country", "/country/city" I've came to the realization that my understanding of array vs objects were still a little shaky. We clarified the concept in detail, along with filtering. Now I am tasked with representing my mental model of my country data structure to represent real-life data structure using arrays & objects.

The response should return with a "country" object for the path "/", a "city" property with an array of cities for the path "/country" and finally Boolean property "visited" that returns Y/N for the path "/country/city". The exercise will be done on a separate .js file so as to not mess up the paths that are already working, then copy & paste onto app.js to test.

berryjen commented 2 years ago

we have used request.params to retrieve the list of hard-coded cities and added additional properties (Boolean, number, strings) to the city object. The response we get back matches to what we coded.

At the moment, the response only returns to we have programmed, meaning, if the user requests for something that isn't coded for, the response won't match the request. Thus, we have created a separate data.json file, where CRUD can be applied to the data structure. This file is where data is stored and manipulated.

My next task is to:

berryjen commented 2 years ago

we made sure that there is consistency throughout the key-pair values ie) there is a name key associated with the specific ethnic group value instead of using the value as the key and the percentage the particular group is comprised of. Also, consistency in values so a string is always a string, a Boolean is always a Boolean ect. we also looked into how to read json data synchronously vs asynchronously and parsing to make sure the content matches the file name. In addition, we noted the difference between filter(broad) vs. find (specific) functions. My next task is to create a function for /:country/:city that will return the city object

berryjen commented 2 years ago

The /:country/:cityfunction I attempted to create almost worked except I had forgotten to take scope into account. We spotted the error and declared the variable result again inside the /:country/:city function. In addition, we added an if statement within the newly created function to account for the condition when a country doesn't exist, the return will be "country not found". We also distinguished the differences between null, undefined and an empty {} in JS. My next challenge is to render a return that will display the correct error status when a country doesn't exist ie) 404 country not found. To challenge the return even further, render a response that will display 404 city not found when the country exists but not the city within.

berryjen commented 2 years ago

We dove into SQL & SQLite (differences and relationship between each other) downloaded the management system library and created a separate db.js file to look at how to manipulate data. We looked in detail at an example code of how SQL works, deciphering line by line and what we think the console will print, and checking the understanding based data structure that it actually displays. We then discussed about edge cases. My subsequent challenge is to take the country& city objects & its associated properties, transform it into an excel spreadsheet , deciding the rows & columns used, how to structure the data. To take it one step further, begin exploring the methods used to create tables using SQLite.

berryjen commented 2 years ago

We created a new file, create_db.sql, going over the methods of creating a table from scratch. I discovered that the BOOLEAN data type doesn't exit in SQLite; however, the management system recognizes the key words True & false, so it "converts" them into 0 & 1 respectively. We have rearranged the spreadsheet data structure to resemble more of an SQL format. I thought that the way data had been structured under data.json was transferable to sql, but the nature of questions asked- return all the data objects without any filtering vs return countries that i have been/ return cities that i want to visit again- are of completely different kinds, so the data structure needs to be re-organized. I will also populate more data into the SQL to cover a range of scenarios. My next task is to write a query to return a list of cities i haven't been but in countries i'd visit again.

berryjen commented 2 years ago

We improved our query column on the spreadsheet, from would visit again to just would visit. The logic is that if I had already been to a country, and I check the would visit, that implies i'd go back again. We then changed the column name in the db.sql file to reflect the improved logic. I ran into the error "no such column "no"" and realized that instead of entering "false" & "true", I had mistakenly put in "no" & "yes" instead. I had originally used

SELECT *
FROM cities
WHERE visited = false
  AND would_visit = true;

to answer the query that will return all cities that i haven't been to in countries id visit again (ie. madrid) as i didn't know/ hadn't learned how to join data columns from 2 separate tables. In order to really test if the query worked, we changed the would visit value of Madrid from true to null. It didn't return. This meant it didn't answer the query we had asked. We then used JOIN with a conditional expression. We had to change the column names that contained overlapping info (country in cities table & name in countries table) into the same to keep consistency & avoid confusion when the columns are joined. We then had to be very specific about which column belonged to which table, so we did it by:

SELECT *
FROM cities
JOIN countries ON countries.country = cities.country
WHERE cities.visited = false
AND countries.visited = true
AND countries.would_visit= true;

The query returned exactly what the query had asked for.

My next tasks are to a) import data containing all countries & cities in the world b) (aggregation) write a query that will return the % of the countries I've been to -- part 2) (limit) return the % of the cities in a country I've been to (ie: % of citieis in England visited) c) (random) return a city to visit next (but not one where i've visited already)

berryjen commented 2 years ago

I added a blacklist column to write the query to generate a random return of a city that I haven't visited, from my populated list. We went over aggregation with the following code:

SELECT visited,
  100* COUNT(country) / (SELECT COUNT(country) FROM countries) AS 'percentage' 
FROM countries
WHERE visited = true
GROUP BY visited;

did a double query in order to find out part of a total and converted it to percentage. This was by far the most challenging query to write as figuring out how to represent the denominator proved to be quite difficult. We also had to "trick" the computer into thinking that it was working with a floating integer instead of just integer, otherwise the result would've resulted in 0. We also had to create several columns to display what we are working with (visited, not visited, the numerator & denominator) and then deleted the columns we didnt need at the end. We also fixed the 3 columns 2 values error message in the countries table by removing the visited column entries (default value= false) from countries that I haven't visited, as it the number data type entries needed to match the number of values entered. The next task is to find a csv dataset that includes all the countries & cities in the world and figure out how to import that data to SQLite.

berryjen commented 2 years ago

Bill had found a Github world CSV dataset that contained codes to extract the columns of info we needed and was better suited than the CSV dataset i had found. We had to add an id column to the countries table & both idand country idcolumns to the cities table because we realized that if we wanted to change the country name in one, we have to change in the other too. Theidcolumns were for combining and importing dataset from external sources. In the original data, we could've used the country abbreviation column for mapping, but because it has real world meaning (if some politician of a country decides to change the abbreviation, then we will have to update the entire data, which is more work) so we chose to arbitrarily assign a number to each country as it has no real world meaning. We used the country columns in both the countries & cities table to join the columns together as that was the only common property between the 2. We also had to update the constraints on the column entries to better reflect the newly imported data. In addition, we also updated the cities table using the following code:

UPDATE
    cities
SET
    country_id = (
        SELECT
            id
        FROM
            countries
        WHERE
            cities.country = countries.country);

to map the country id to the cities country id columns together. My next challenge is to remove the countries column from the cities table ( we no longer needed it as now we have mapped each city to its unique city id number and imported the country id over from the countries table) and rename the joined query.

berryjen commented 2 years ago

When I attempted to remove the cities.countries column and rewrote the JOIN query to

SELECT *
FROM cities
JOIN cities.country_id = countries.id
WHERE cities.visited = false
AND countries.visited = true
AND countries.would_visit = true;

and ran the whole query page from top to bottom, the error message near"=" syntax appeared. At first glance, I didn't know exactly where the location of that error is coming from, so I selected each individual query to run to further debug. When I did that, the errors UNIQUE constraint failed: countries.country, cities already exists. After discussing this with Bill, I discovered that I had a misconception regarding the start & end of a query. I had mistakenly thought that if a query is dealing with the same table, as below:

DROP TABLE IF EXISTS cities;
CREATE TABLE cities (
  id INTEGER PRIMARY KEY AUTOINCREMENT,
  city TEXT NOT NULL,
  country_id INTEGER,
  visited BOOLEAN DEFAULT false,
  would_visit BOOLEAN,
  blacklist BOOLEAN DEFAULT false
  );
  INSERT INTO cities (city, visited, would_visit) VALUES ('Madrid',false, null);
  INSERT INTO cities (city, visited, would_visit) VALUES ('San Sebastian', true, true);
  INSERT INTO cities (city, visited, would_visit) VALUES ('Zagreb', true, false);
  INSERT INTO cities (city, visited, would_visit) VALUES ('Split', true, true);
  INSERT INTO cities (city, would_visit, blacklist) VALUES ('Riyadh',false, true);
  INSERT INTO cities (city, would_visit, blacklist) VALUES ('Pyongyang', false, true);
  INSERT INTO cities (city, would_visit) VALUES ('Lima', true);
  INSERT INTO cities (city, visited, would_visit) VALUES ('Lisbon', true, true);
  INSERT INTO cities (city, would_visit) VALUES ('Nazare', true);
  INSERT INTO cities (city, visited, would_visit) VALUES ('Berlin', true, false);

then this entire block belonged to the same query ( a query about the cities table). I hadn't realized that ; indicated the end of a query. So when I placed my cursor within the CREATE TABLE cities line, the error message cities already exists appeared because the CREATE table & DROP table are 2 separate queries! This led to the topic about destructive data (anytime you are writing data). We then restructured our logic for sequencing the events for CSV data import, joining the 2 tables, then deleting countries.cities column. What we realized was that the constant change of data structure, the modifications before & after data import turned out to be a mind game and was giving us more confusion a clean source of data to work with. As we found that importing the CSV data columns we needed to match our final table structure and then having to delete extra info proved to be more cumbersome, we made the decision to improve our method to the following:

  1. create a new, intermediary cities table with columns matching exactly as the CSV file
  2. import that CSV file directly into the intermediary cities table
  3. repeat steps 1-2 for countries table
  4. create cities table
  5. create countries table
  6. write query to copy data from imported over to the desired countries table format
  7. repeat step 6 for cities table format This will be my subsequent task as well as fixing the syntax error on the JOIN query.
berryjen commented 2 years ago

After my attempt of creating the 4 new tables, i had trouble displaying on TablePlus. I knew the tables were created because I ran the queries, but the new tables still didn't show. Thus I had trouble importing the CSV data onto the imported_data table. I came to my wits end reasoning why the tables weren't displaying, which I then discussed with Bill. He pointed that it was as simple as refreshing the view! (In hindsight, I felt so dumb). I was new to using TablePlus which resulted in the juvenile mishap. Bill had pointed out that he was able to see an error in my tables code. I subsequently ran it and `SYNTAX error near ); appeared and I logically reasoned what I know it means and the location. I went from the top of the code block and proceeded down.

DROP TABLE IF EXISTS cities_import;
CREATE TABLE cities_import (
  country TEXT NOT NULL,
  accent_city TEXT NOT NULL,
  population INTEGER,
  latitude FLOAT,
  longitude FLOAT,
);

From existing knowledge, I knew that the most common areas for SYNTAX error were missing , ; opening & closing brackets, spelling errors, so those were what i looked for to start. ~ The first line looked syntactically correct. ; followed the end of a query ~ closing & opening brackets matched ~ a common at the end of each code line signalling another column was to be created ~ then i looked at the last line, there was an extra , at the end of that code line. There was no additional column following after, so I knew that was where the location of the error was. ~ which i then repeated the same process to the countries_import, and had also spotted identical SYNTAX ERROR ~ to improve on the efficiency of error finding, next time i will begin literally where the error is indicated in the message and use that as a stronger hint. We also re-assessed the data type for both longitude & latitude from integer to float as the numbers after the decimal point matters 100%, but we didn't put any constraints because we don't want a city to be rejected just because there isn't a long & lat available. This led to a discussion of when to discard/keep data. If the problem presented is a very specific one and the questions that need to be answered are also of very narrow domain, then discard early. However, if the answer is "i don't know", then it is better to be safe then sorry and keep the data along until the scope of the problem has been narrowed down further.

We then inserted the specific columns we wanted into the final cities & countries tables from the import tables. The value insertions into the cities table proved to be the most cumbersome as multiple accounts of the name country needed to be discerned as well as involving a JOIN query.

This concludes the SQL/SQLite section of the travel API. The real- world database is finally ready be used. We will move the stage back to Javascript, writing queries in JS to return countries & cities. My subsequent tasks are to: 1) write query to return all countries 2) console.log the country return 3) return all cities 4) return all cities from any country 5) return all cities from a specific country (Tunisia in this case & this requires documentation look-up)

berryjen commented 2 years ago

After attempting to write JS function returns and console.log the results, nothing appeared in the terminal. I realized that I had called on the wrong file (node app.js) so as my first step of debugging, I typed in the correct one (node db.js). Nothing was displayed still. Upon our pair programming session yesterday, I had discovered that I didn't terminate (control + c) the previous call so the terminal wasn't able to handle my new command. I learned what terminal prompt looks like for my console (Bill pointed out that it can customized!) The errors then appeared. We went over the concept of function signature, where the data type, order in which it is entered, and the amount of parameters all need to stay the same as the function given.

db.each('SELECT rowid AS id, info FROM lorem', (err, row) => {
    console.log(`${row.id}: ${row.info}`)

I knew it was a generic function so i had mistakenly thought that you can "customize" the parameters (in this scenario, country). Also, => is the most recent convention for representing function , which I didn't know about. I had also mistaken back ticks``as single string '', which, in hindsight, was so foolish of me. I had also learned to make the best use of the color coding system VSC offers as clues for debugging when the colors don't match up. Going over the db.each function in great detail, I now understand that this particular function takes the first parameter as a string'', then the second parameter is a function with specifically the parameters (err, row). I had to mentally translate /map the example row parameter to our country rows. Also, at some point we will need to add an IF statement to handle the error parameter, should rows not exist for some reason.

db.each('SELECT country FROM countries',
    function(err, row) {
      console.log(`${row.country}`) 

The above code now displays all the countries on the terminal. My next challenge is to fix the cities & cities from any country functions

berryjen commented 2 years ago

The city db.each was an easy fix as it follows exactly the country db.each pattern. The cities from any country db.each was way more challenging. I had learned that the order of operations in SQL is not top to bottom, left to right, as how all codes are written. Instead, the sequential order occurs in a similar fashion to math operations. For example, multiplication & division before addition & subtraction, anything inside the brackets are performed first etc.

db.each('SELECT city, country FROM cities JOIN countries ON cities.country_id = countries.id WHERE country= ?', requested_country, 
    (err, row) => {
         if (err) {
          console.log(err)
         }
         else {
          console.log(`${row.country}: ${row.city}`)
         }
    }
   )

The major source of my confusion stemmed from the erroneous notion that the SELECT query literally executes as it is written top to bottom. Instead, the JOIN happens first, then FROM, then WHERE, then finally SELECT. I was so confused about the WHERE clause as the citiestable did not contain the country column but yet we are JOINING countries from 2 separate tables and selecting from a table that doesn't have the per-existing column we required. Turned out it is SELECTING from a newly JOIN table. We had an error displayed that read cannot read property 'country' undefined. We couldn't see what type of error it was (syntax etc) so we had to write an IF statement that handled the error parameter as well as console.log the error so we know what the error message says. Before we tried callingconst requested_country= 'Tunisia', we hard coded Great Britain to see if our function worked. It threw yet another error as I had forgotten the parameter only takes a string and didn't put'' around the text. When I did that, the error still persisted. My debugging logic was as follows: 1) i checked the spelling of the string against the countries table. It was correct 2) I remembered that I had fixed the way some of the country names were presented so I deleted the unwanted part only to forget that I had left a space after the last character 'Great Britain '. It was then that Bill pointed out that it had to be an EXACT match 3) went back to the function and left a space before the closing string, which did fix the problem. This turned out to be more cumbersome so I fixed it at the source and removed the space from the table. We then searched the syntax for embedding a constant within a JS function, which was putting a '?' and then referencing therequested_country const which was declared at the very beginning.

My next task is to: 1) filter countries using LIKE query (to get a partial match) 2) each API call has a different query in app.js......what SQLite query do I want to write to return a specific country with a city found within it eg) Great Britain, London

berryjen commented 2 years ago

Writing the LIKE query was relatively easy and I had debugged the errors independently. One of the the first error was that there was something problematic with db.serialize function, but I knew i hadn't tinkered with that at all, and also knowing that the previous function had worked just fine (requested_country). Upon further inspection, there was a syntax error with LIKE so i goggled SQLite documentation to fix it, then realized that i had 2 sets of the same quotes ('')around the SELECTquery & around the LIKE parameter. I changed the quotes to one set of single and one set of double, which solved the issue. The specific SELECT query to return a specific country and its corresponding city was very similar to the query that returns a specific country with all of its corresponding. The only additional things were declaring a new constant for city at the beginning of the file, and adding city = ? and calling the constantrequested_city. I was having trouble debugging the syntax error when I attempted writing it myself and hadn't clued in that there is a difference between & bitwise vs AND logical operators, which Bill had kindly pointed out.

const requested_country= 'Tunisia'
const requested_city = 'Tunis'
db.each('SELECT * FROM cities JOIN countries ON cities.country_id = countries.id WHERE country = ? AND city = ?', requested_country, requested_city,
    (err, row) => {
         if (err) {
          console.log(err)
         }
         else {
          console.log(`${row.country}: ${row.city}`)
         }
    }
   ) 

We then discussed in depth the difference between app.js & db.js files and how they interact differently with the database to fetch data for users. App.js is the server itself that talks to the browser, which is listening on a local port, always on standby, waiting for a GET request to be executed. When a user makes a GET request, it then process the request, talks to the database, and then displays the response on the browser. Everytime you update the code, you have to terminate the server in order for the new change to take effect. Db.js is not a server and does not talk to the browser in any shape or form. We write the queries, let the codes run, which in turn talk directly to the database, returns the response on the console. It has no local port to talk to. It takes commands directly from the terminal and there is no need to "terminate" anything. Just re-type the file name for the change to take effect. We then modified our app.get('/') function based on thedb.each function so that it would only returns one response at a time. The difference being that instead of returning each row of country individually, return a massive array containing all of the country objects, returned all at once. We also took error handling into account where we wrote an IF ELSE statement displaying the type of error on the console, as well as the error message on the browser that will be shown to users on the frontend.

app.get('/', (req, res) => {
  db.all('SELECT country FROM countries',
    function(err, rows) {
      if (err) {
          console.log(err)
          res.status(500).send("An error occurred here")
         }
         else {
          res.json(rows)
         }
    } 
  )
});

My next challenge is to modify theapp.get('/:country') to return all the cities.

berryjen commented 2 years ago

Upon discussing my modified db.all function for ('/:country')I realized that it was returning all of the cities to all of the countries (which also slowed the performance). We first tested it out with a hard coded country const= 'Portugal'to ensure that our code was working, and it did. Next, we had to figure out how to store the country the user requested in a variable. In one of the previous functions we wrote at the start of the project, we discovered that req.params.country was one way to take "live" request. An error 'request undefined'displayed and I over analyzed it. Bill had kindly pointed out that this message was being as direct as possible, and hinted to look at how i wrote 'request'. Instead of 'req', i typed 'request', which didn't match the arguments given in the function. When that was corrected, we tried it 'Columbia' and it returned an empty array '[ ]'which i immediately thought was weird. The country actually exists, and i know our database contains such info, so I suspected a spelling error was the cause. This was the case as it was 'Colombia' instead. Because of this scenario, we took the opportunity to further refine the error handling function. Technically speaking, there was nothing wrong with the code; it was doing its job. We came up with else if (rows.length === 0) vs else if ([ ] === 0) and discussed the readability & understandably between the 2. The former is explicit and using info that is already contained within the database (length of each country). The latter involves an empty array being created first and them do the comparison, which takes up memory & decreases performance.

app.get('/:country', (req, res) => {
  db.all('SELECT city, country FROM cities JOIN countries ON cities.country_id = countries.id WHERE country= ?', req.params.country, 
    (err, rows) => {
      if(err) {
        console.log(err)
        res.status(500).send("An error occurred here")
      }
      else if (rows.length === 0) {
        res.status(404).send("Country not found")
      }
      else {
        res.json(rows)
      }
    }    
  ) 
}); 

We also created new search indexes for country, city & country_id to increase speed performance as the wait was too long.

CREATE UNIQUE INDEX idx_countries_country
ON countries (country); 

CREATE INDEX idx_cities_city
ON cities (city);   

CREATE INDEX idx_cites_country_id
ON cities (country_id); 

My next task is to create db.all to return a specific country & city. To take it a step further, return (SELECT) all the useful cities table columns

berryjen commented 2 years ago

Creating db.all to return a specific country & city was easy as it followed the same pattern There was an error relating to the query and I immediately knew that i had forgotten to add the AND city = ? as well as req.params.city. The challenge came withSELECTINGuseful columns we wanted to display for the users. When i tried to tinker with it at first, I've copied & pasted the same function from the one with specific country & city one, SELECTED the useful columns, only for the response to return exactly as the function above. I debugged it by changing the function path to app.get('/:country/:city/info') . This solved the behaviour as it was the same path before and the server didn't know which path to run. Difficulty arose when I tried to SELECT specific columns to return with the error ambiguous column name. I attempted to fix it by changing the query to SELECT * This seemingly worked as it had returned all the columns, or so I thought. I was confused as to whySELECT * worked while SELECTING specific columns resulted in an error. After discussing this issue with Bill, it turned out that I was unsuccessful in noticing there were 2 columns with the same name from 2 different tables, and my instruction was not explicit. When we ran the SELECT * query in TablePlus, it had returned 10 columns in total, where as on the browser, only 7 were displayed. What this told us that SELECT * wasn't fully functional either due to the fact that the terminal didn't know which of the duplicated columns to return from which table. 2 solutions were proposed as follws: 1) follow the table.column convention to specify the column & table pros: easy fix, no additional code required cons: doesn't allow the natural hierarchy of representation of the real world. Instead it's displayed as a key-value pair 2) change data structure pros: returns data structure that reflects a better representation of the natural world

In the end, we made the decision to opt for solution 1) as for the time being, there is no need to change the existing data structure. In the future, however, there will be a need to change it to better serve the need of the users. This is a deliberate and prudent tech debt as we want to see some immediate result & ship the code to get a working API up and running within a reasonable time. The consequences of these decision will be dealt with later when the time arises. app.get('/:country/:city/info', (req, res) => {

db.all('SELECT city AS name, country, cities.visited, cities.would_visit, cities.blacklist FROM cities JOIN countries ON cities.country_id = countries.id WHERE country= ? AND city = ?', req.params.country, req.params.city, 
    (err, rows) => {
      if(err) {
        console.log(err)
        res.status(500).send("An error occurred here")
      }
      else if (rows.length === 0) {
        res.status(404).send("Country not found")
      }
      else {
        res.json(rows)
      }
    }    
  ) 
});

I thought to extend my current knowledge & understanding of error handling to further incorporate an additional error IFstatement to account for city.

else if (rows.length === 0) {
        res.status(404).send("City not found")
      } 

Turned out, I had forgotten about the 1 request 1 response rule which meant that the server wouldn't know which error message to print out as both contained identical code.

The subsequent challenge for me is then to: 1) return an additional column for the countries table 2) start writing a function an app.postrequest

berryjen commented 2 years ago

We discussed the differences between app.post vs app.get and situations/circumstances where one would want to use one over the other. This ties in with the HTTP methods and CRUD, and I learned that these are 2 separate entities. The former is a more practical approach dealing with implementations, while the latter is conceptual/ a mental model and is derived from engineering practices. One can use CRUD to explain HTTP methods but there are no hard -lined match between the 2. We used a simple example of

app.get('/drink', (req, res) => {
  console.log(req.query.temperature)
  res.send('Here is your ' + req.query.temperature + ' drink')
}); 

to investigate req.query.params and the logistics of how it works via Curl & an app called Postman, which simplifies command line codes & is visually easier to understand the process from beginning to end. When we ran that function, the response didn't match our expectations. It returned results relating to the app.get('/') function. We debugged it by placing the app.get drink function at the very beginning of the code and it worked. Turned out that it was an issue related to order of route handlers. Any time agetrequest is made, req.query.params is always used. As this data gets logged, it is important to validate the data that users provide to make sure that no malicious info is given to try to hack database etc. Key-value pairs are used to store the query params. We then investigated the nature of app.post using the following function:

app.post('/drink', (req, res) => {
  console.log(req.body.type)
  res.send('Thank you for the ' + req.body.type )
});

before the method can be called, middleware function needs to be defined (once) first outside of the app.postscope. It runs between the incoming request and before roue handler gets called in order for authentication purposes. Whenever app.post is called, req.body.params is always used. It provides a JSON body object for transferring purposes. It doesn't get logged. My follow-up question would then be: why is the middleware function used for app.post and not app.get?

We extended our knowledge and applied it to the app.get('/') function as follows:

app.get('/', (req, res) => {
  var query = 'SELECT country FROM countries'
  if (req.query.visited === 'true') {
    query = 'SELECT country FROM countries WHERE visited = true'
  }
  else if (req.query.visited === 'false') {
    query = 'SELECT country FROM countries WHERE visited = false'
  }  

  console.log (query)

  db.all(query, 
    function(err, rows) {
      if (err) {
        console.log(err)
        res.status(500).send("An error occurred here")
      }
      else if (rows.lenghth === 0) {
        res.status(404).send("Countries not found")
      }
      else {
        res.json(rows)

      }     
    } 
  )
});

The subsequent challenge for me is to implement additional SELECT quires to handle combinations of query string params. To take it one step further, render a new app.post function ('/:country/visited') that will return a JSON object with useful info included.

billglover commented 2 years ago

Clarification on the use of query parameters:

"Any time agetrequest is made, req.query.params is always used. As this data gets logged, it is important to validate the data that users provide to make sure that no malicious info is given to try to hack database etc."

When a user (or an app) makes a request for an HTTP URL, this request is handled by a server. It is common for servers to log these requests for analytics (e.g. How many people visited /uk/london?) or debugging (e.g. Which URLs result in a HTTP 404 status response to users?). It is common for these logs to include everything in the query string (e.g. /uk/london?user=bill). We need to be mindful of the fact that these logs are likely going to contain values in the query string and avoid putting information there that we may not want logged (e.g. user name).

But this behaviour is a side effect, of how logging has been implemented on servers. A more semantic way to think about this is to consider what a URL is, and what the body of a request represents.

A URL is short for Uniform Resource Locator, it points to the location of a resource (or object) being requested. All the parameters in the query string form part of the URL and should help identify the resource being requested. So we could have /countries to point to or locate a list of all countries. Or we could have /countries?visited=true to point to or locate a list of all countries that we have visited.

The body of a request (and response) contains the resource that has been requested. In the case of a GET request it contains the resource (or object) being transferred from the server to the browser. In the response of a POST or PATCH it contains the body of the resource being transferred from the browser to the server.

berryjen commented 2 years ago

The answer to my previous question "why is the middleware function used for app.post and not app.get?" is that it can be used for both. In the former instance, middleware is used to validate the object passed within the body, and the latter checks the validity of the query params passed. When i checked in the browser to see if my original query statements of SELECT country FROM countries WHERE visited = true & would_visit = true & SELECT country FROM countries WHERE visited = false & would_visit = false, both returned identical results. This shouldn't have been the case, and I hypothesized that it was because we had set the default values in SQLite to be false & null. Turned out that I had failed to include the correct query params in the address bar ?=params=value&param2=value2. When that was fixed, the response was still the same. This brought our attention to my query statements. Instead of having to write multiple If & query statements to account for all the possible combinations (9 in total which is tedious work), Bill had kindly pointed out a "shortcut" which was to store the values of visited, would_visit & blacklist inside a variable and default their values to false, as logically speaking, people haven't been to most parts of the world, thus returning only a subset of the entire country object. This way, we would only have to write the condition for true, check for it, and again returning only a subset of the entire array. We updated the IF statements as follows:

app.get('/', (req, res) => {
  var visited_query = 'false'
  if (req.query.visited === 'true') {
    visited_query = 'true'
  }
  var would_visit_query = 'false' 
  if (req.query.would_visit === 'true') {
    would_visit_query = 'true'
  }
  var blacklisted_query = 'false'
  if (req.query.blacklist === 'true') {
    blacklisted_query = 'true'
  }

The next logical step is to update the query statement that will reflect the aforementioned IF statements that handles whatever user request will be (instead of the values being hard coded). The most efficient way to do this is to concatenate the query statement strings with the 3 variables. I had no difficulty understanding it conceptually but had the most challenging time physically concatenating the strings and variables together. My theory is that the composition of the concatenation was more complex and my brain, as a result, stuttered. The final query statement is as follows: `query = 'SELECT country FROM countries WHERE visited = ' + visited_query + ' & would_visit = ' + would_visit_query When we updated the query params in Postman, the response displayed was an empty array.

My subsequent challenge is to debug why the result returns an empty array

berryjen commented 2 years ago

Upon creating the req.query.blacklisted variable & IF statement, i\I’ve realized that we never included the blacklist column in our countries table, which meant that our blacklist query wouldn’t be of use. I came to this realization after i had concatenated blacklisted_query onto the string, to which the response came back with an error of SQLITE_ERROR: no such column: blacklist Meaning that we will either have to delete the blacklisted_query from the app.get('/') function, or paste it into the app.get('/:country') function, or add the blacklist column directly onto the table on TablePlus. After discussing it with Bill, I decided to add the blacklisted column onto the countries table as it made more logical sense to blacklist an entire country vs having the ability to blacklist individual cities. I updated the SQLite query statements in both VSC & TablePlus, however in VSC there was an error of "duplicate columns" but I couldn't see the changes in TablePlus after refreshing the page, which was strange. Originally I added the blacklist column under the CREATE TABLE countries, and set data type & default value. I then changed it to

ALTER TABLE "countries" 
  ADD COLUMN "blacklisted" Boolean DEFAULT false;

and the same error message still popped up. We then closed the app and restarted, and it worked. I deleted the duplicate column, ran the updated SQLite queries and had to reinstate the INDEX queries to speed up performance.
When we tinkered with the query params in PostMan, the results were empty. We cross checked with the console.log(query) to see exactly which query was being executed. We then changed the default value visited_query = null , but it still didn't return anything. As we discovered, null was a special keyword that needed to be capitalized. We manually tested out several query scenarios, and thus had to physically change the values for visited, would_visit & blacklistedin order to see if the results were what we expected. In the end, we had to change the default values for all 3 variables and check both true & false conditions to account for all combinations

app.get('/', (req, res) => {
  var visited_query = 'NOT NULL'
  if (req.query.visited === 'true') {
    visited_query = 'true'
  }
  else if (req.query.visited === 'false') {
    visited_query = 'false'
  }

NOT NULL all had to be capitalized as they are part of the special keywords. It also accounted for the fact that the value couldn't be both true or false individually for each country. However, when req.query.params are combined, the final result can contain visited values of both true and false for countries that hold as such. This meant that we had to write an IF condition to check for both true & false.

var would_visit_query = 'NULL OR would_visit IS NOT NULL' 
  if (req.query.would_visit === 'true') {
    would_visit_query = 'true'
  }
  else if (req.query.would_visit === 'false') {
    would_visit_query = 'false'
  }
Setting the default value for this was trickier as when we tested out only NULL or NOT NULL individually, the results didn't include all of the countries we had expected. For any given country, one could either want to go back again (after having visited already) or want to go , but hasn't ever visited, or simply have no opinion on it. Thus, we had to include both values in 1 variable. 

Finally,

var blacklisted_query = 'NOT NULL'
  if (req.query.blacklisted === 'true') {
    blacklisted_query = 'true'
  }
  else if (req.query.blacklisted === 'false') {
    blacklisted_query = 'false'
  }

For consistency purposes, the value was also changed to NOT NULL as it must have either a true or false value. After implementing the changes, some of the behavior were still not what we had expected. We debugged it by examining the query statement itself, as some of the statements being run against the query params didn't make sense. Turned out there is a difference syntactically in SQLite between IS / = . x is null checks whether x is a null value. x = null is checking whether x equals NULL, which will never be true This debugging approach still didn't quite return the result we anticipated. Bill remembered that there was a similar problem we encountered a while ago that dealt with the order of operations. We added () around the statements that we wanted to be done first before ANDis applied. query = 'SELECT country FROM countries WHERE visited IS ' + visited_query + ' AND (would_visit IS ' + would_visit_query + ') AND blacklisted IS ' + blacklisted_query This finally took account of every combination among visited, would_visit &blacklisted!

While debugging, this led to a discussion about manual vs. automated testing as well as deployment vs production.

There are 3 challenge variations for my next step: a) consolidate IF & query string parameter filters to app.get('/:country') --> apply visited, would_visit to the list of cities b) app.post --> to not manually update info on the database --> have the function do the work c) error handling for app.get('/') --> for unexpected values --> which error code to return ex) ?visited=France

berryjen commented 2 years ago

Upon attempting to apply IF & query string param filters to app.get('/:country'), problems started to arise when I try to alter values for visited & would_visit columns in the cities table. The error attempting to write over a READONLY file popped up. We had to, like last time, close the app, re-import CSV files, re-inserting the tables, and re-indexing certain columns. The reason for this, as Bill noticed, was due to the fact that content changes inlorem.db was pushed onto GitHub every time the code was committed, meaning it was being tracked. TablePlus is accessing the same database that I'm trying to make changes to simultaneously. To debug this fully, we had to create a new database called world,transferred contents from lorem, delete lorem, change the source of db to world, unchecked the tracking box for committing, closed & re-opened Visual Studio for the changes to take effect. Then we had to debug why the query was returning error. We took a look at the query that was logged in the console, and discovered the closing bracket) was placed incorrectly. We had to concatenate ) at the very end of query. city_query = 'SELECT city, country FROM cities JOIN countries ON cities.country_id = countries.id WHERE country = ? AND cities.visited IS ' + visited_city_query + ' AND ( cities.would_visit IS ' + would_visit_city_query + ')'

The next source of where the error originated was from db.all function. I forgot to pass the query variable created earlier in the function as part of the arguments, and storing the user input value in req.params.country.

app.get('/:country', (req, res) => {
var visited_city_query = 'NOT NULL'
  if (req.query.visited === 'true') {
    visited_city_query = 'true'
  }
  else if (req.query.visited === 'false') {
    visited_city_query = 'false'
  }

  var would_visit_city_query = 'NULL OR cities.would_visit IS NOT NULL' 
  if (req.query.would_visit === 'true') {
    would_visit_city_query = 'true'
  }
  else if (req.query.would_visit === 'false') {
    would_visit_city_query = 'false'
  }

  city_query = 'SELECT city, country FROM cities JOIN countries ON cities.country_id = countries.id WHERE country = ? AND cities.visited IS ' + visited_city_query + ' AND ( cities.would_visit IS ' + would_visit_city_query + ')'

  console.log(city_query)

  db.all(city_query, req.params.country,
    (err, rows) => {
      if(err) {
        console.log(err)
        res.status(500).send("An error occurred here")
      }
      else {
        res.json(rows)
      }
    }    
  )
}); 

We discussed potential improvement features to add to the function, such as:

We then moved on to creating a new app.patch(':/country') function that handles updates. The differences between post & patch were also noted while completing the exercise. This was especially challenging for me as I had to figure out where to putconsole.log, validating data type, writing elaborate IF statements (and sorting out the sequence), updating the query statement, creating a newdb.run function that accounts for all the column values & storing user input for country, and finally updating the error handling function as well.

app.patch('/:country', (req,res) => {
console.log(req.params.country, req.body)

var visited = true if (req.body.visited === undefined) { res.status(400).send("invalid input") return } else if (req.body.visited === true) { visited = true } else if (req.body.visited === false) { visited = false } else { res.status(400).send("invalid input") return }

query = 'UPDATE countries SET visited = ?, would_visit = ?, blacklisted = ? WHERE country = ?'
  db.run(query, visited, req.body.would_visit, req.body.blacklisted, req.params.country, 
    (err) => {
         if (err) {
          console.log(err)
          res.status(500).send("update unsuccessful")
         }
         else {
          res.status(202).send("accepted")
         }
    }
    );
});

We console logged at the very beginning because we wanted to see the values users passed into the query params(country & properties of the city) for debugging purposes. We then continued to write the IFstatements for the visitedvalue. It made logical sense to catch the undefined values first (non-existent country), then true & falseconditions, then finally everything else. We also used UPDATE in the query and set all the column values & country to = ? (as we don't know what values the user will give us) . The row argument had been removed from the error function as we don't need to catch which country row the error is coming from, as the user will specify that. The error message had also been updated to 500 to account for the server side not working at this point, after all the IF statement checking.

My subsequent challenge is to write out the IF statements for would_visit, blacklisted, & country.This will hopefully drill in theIF syntax. As an extension, I will attempt to think of ways to refactor all these IF conditions into 1 single function that will take care of checking BOOLEANvalues.

berryjen commented 2 years ago

To refactor all the elaborate IF statements, we have created a new function called validate boolean. We dissected the parts that a function requires and decided to name the argument value, as it takes user values. Then we looked at the 4 IF scenarios we have writtien: invalid country names, return undefined, true return true, false return false, everything else (numbers, symbols etc) return undefined. We were able to transfer the same logic across as the following:

function validate_boolean(value) {
  if (value === undefined) {
    return undefined
  }
  else if (value === true) {
    return true
  }
  else if (value === false) {
    return false
  }
  else {
    return undefined
  }
}

Bill proposed the option of further refactoring by removing

else {
    return undefined
  }

because essentially, the only 2 values we are accepting are T/F so anything else outside of that, it will return undefined. I've decided to not further refactor as based readability & understanding, as in, anyone seeing this code for the very first time can efficiently reason through. Next on the list to improve is error message displayed on the browser. Reasoning through the logic was challenging as we couldn't possibly know in advance which input the users will provide, which was a rabbit hole in itself. In the end, we decided to create a variable to store all errors in an array. Then, we appended the second half of the error message, for each particular filed, in string format, onto the end of the array. We needed to join the array together with the first half of the error message.

app.patch('/:country/:city', (req,res) => {
  console.log(req.params.country, req.params.city, req.body)

  var boolean_errors = []
  var visited = validate_boolean(req.body.visited)
  if (visited === undefined) {
    boolean_errors.push("visited (required, boolean)")
  }
  var would_visit = validate_boolean(req.body.would_visit)
  if (would_visit === undefined) {
    boolean_errors.push("would_visit (required, boolean)")
  }

  if (boolean_errors.length > 0) {
    res.status(400).send('Validation failed : ' + boolean_errors.join(', '))
    return
  }
  query = 'UPDATE cities SET visited = ?, would_visit = ? WHERE cities.city = ? AND cities.country_id IN( SELECT id FROM countries WHERE country = ?);'

  console.log(query)

  db.run(query, visited, would_visit, req.params.city, req.params.country,
    (err) => {
         if (err) {
          console.log(err)
          res.status(500).send("update unsuccessful")
         }
         else {
          res.status(202).send("accepted")
         }
    }
  );
});

Our next new feature will be focused on writing tests. This is an area I haven't worked on much at all, other than the very brief intro to Rspec in Ruby testing. The aim is to write automated tests that will take care of edge cases and ensures our code behaves how it's supposed to. After a brief research, Jest came up quite frequently and seemed easy to install & get running off the ground (beginner friendly). I will set this up for the next session.

berryjen commented 2 years ago

I successfully installed jest and ran the sample test example. I debugged a few errors independently by reading the error messages No test specified. At first i thought it was the syntax (missing, ; () {} etc) but the errors still persisted. I then realized that I had named the file wrong (sum.js.test vs sum.test.js), upon which the same error message still persisted, so I took a look at the json package objects and realized that there were 2 objects with the same name, deleted the one that wasn't in use. Debugging completed. Now the real challenge was reasoning through the test itself.

const sum = require('./sum');

test('adds -1 + -7 to equal -8', () => {
  expect(sum(-1, -7)).toBe(-8);
});

because the => notation was used in lieu of a function, it was difficult for me to tell where the second param began and ended. Also, it was very important to name parameters explicitly as the order in which the results are logged on the console could be different than the sequence the tests were fed in. In addition, often times the person defining/writing the function is different from the person using/calling it, so argument names are, in this case, irrelevant for as long as the function signature remains the same. To solidify the concept, we did a separate exercise of calling a function within a function and then defined that function outside, each with different argument names passed in.

function sum(a, b) {
  make_meal("dinner", function instructions() {
    console.log("step 1")
    console.log("step 2")
    console.log("step 3")
  } )
  return a + b;
}

function make_meal(meal, recipe) {
  console.log("I am making " + meal)
  console.log("retrive ingredients") 
  recipe() 
  console.log("clean up")
}

Where I called make_meal at the top and passed it different argument names (dinner, function instructions) (where this function takes on another function as its second parameter) and bill defined function make_mealusing his own argument names (meal, recipe). I reasoned that function sum takes in 2 arguments called a, b and inside the block of code takes another function that takes 2 arguments, first being a string and second being another function called instructions which takes an empty parameter. Function instructions is called, which invokes function make_meal outside the scope. The function takesmeal & recipe (which is the same asinstructions). The block of code inside will print whatever is logged. In the first instance, it will display the string I am making + the meal, which is the dinnerparameter passed into the function when it's called. Next terminal will display retrieve ingredients, then calling recipewith empty parameter, which is the same as function instruction. It will then print out step 1-3, exits function, then finally displayclean up. The subsequent challenge is to use the make_meal() function to request breakfast, provide a new recipe. Extra challenge is to modify theinstructions() function (and the places we call it) so that we pass in the number of portions we want to make. Use this number of portions to modify the instructions

berryjen commented 2 years ago

Upon doing the challenge extension exercise, I think we have pinpointed where the blockage is coming from. The source seems to be coming from recognizing when a function is defined vs when it is called (executed). In order to change the portion size, we first had to change the parameter to function instructions so that it is no longer empty and takes amount as a new argument. Also, we needed to call theamount argument in one of the console.logto reflect the parameter change made in the function.

make_meal ("breakfast", function instructions(amount) {
    console.log("chop garlic, onions, veggies of choice and sautee everything")
    console.log("fry " + amount + " eggs")
    console.log("season with salt, pepper and herb mix of choice")
    console.log("toast bread, use spread of choice")
  }

We then also had to update how we call(execute) the make_meal function to account for taking in serving portions. Because we didn't want to hardcode the amount, we needed to add a third argument into the function, called servings. We then had to pass servings intofunction recipe (which callsfunction instructions) as the new argument. Because we defined a new parameter in make_meal, we also had to pass a new value for the function to accept, as the 3rd argument, after function instructions.

function make_meal(meal, recipe, servings) {
  console.log("I am making " + meal)
  console.log("retrive ingredients") 
  recipe(servings) 
  console.log("clean up")
}
make_meal ("breakfast", function instructions(amount) {
    console.log("chop garlic, onions, veggies of choice and sautee everything")
    console.log("fry " + amount + " eggs")
    console.log("season with salt, pepper and herb mix of choice")
    console.log("toast bread, use spread of choice")
  }, 4)

We investigated further into writing our own tests and decided that it would be more beneficial to separate app.js from the server so that we can just test the functionality without having to go through the browser. This was achieved by creating a new server.jsfile, transferred the server code lines over. I decided to go with async way of testing as it is something i've had a bit of familiarity with. A new testing file called async.test.js was created where the actual bulk of the tests will be contained. The source of the error was coming from getting our file path correct.

const request = require("supertest");
const app = require("./app.js");

describe("Test the root path", () => {
  test("It should response the GET method", async () => {
    const response = await request(app).get("/");
    expect(response.statusCode).toBe(200);
  });
});

const response = await request(app).get("/"); this code looks almost identical to our app.get('/') request, which means that it is testing for a list of countries to be returned. expect(response.statusCode).toBe(200) if our code matches with the behavior we expect, which is to return a list of countries, then the status code will be 200 ok. My next challenge is to use the existing test to test the get request for /Croatia with a response code of 200. On top of that, test the get request for /Mars with a response code of 404.

berryjen commented 2 years ago

We dissected the structure of writing tests in Jest, moving towards our first step of incorporating TDD (test driven development) into part of our coding process. Testing is done so that programmers can describe the desired/ observed behavior in a system or an app. The code describes the steps (how-tos) required to achieve the desired behavior. Bill gave the following example to illustrate the differences:

If any of the "code" is wrong, then it will not return the expected results. It is easier to describe the desired behaviour and by doing so, the outcome of all the codes are validated rather than the specifying exactly the one set of instructions to get the task completed. This is backwards design. One begins with the outcome, then plan backwards & work towards the steps on how to produce the desired outcome. Testing can also be used as a communication tool. It is easier to converse with non-tech people about desired system behaviour rather than code implementations; e.g. It should return a list of all countries, or return a list of country i would visit but haven't been etc. This allows us to communicate concisely when writing tests that can easily be understood in English and validated against desired system behaviours. Across all programming languages, it is common occurrence for testing frameworks to allow engineers to to structure tests so that they can be easily comprehended.

GET /:country
* should be successful (200)
* should return a list with >0 cities
* should include a `visited` property
* should include a `would_visit` property

Is the example we are practising with. The first 3 outlined above have been completed by writing the "description" and the "tests" so they read as close to natural English as possible. My subsequent challenges are to:

berryjen commented 2 years ago

We grouped the tests according to function, so in this case, a test for GET request /:country. in the describe function. This specifies the umbrella group of tests. Writing the test description was easy as it is in verbatim English; this specifies the single test to be carried out.

test('should respond with a non-empty list of cities', async () => {
        const response = await request(app).get('/Italy');
        expect(response.body.length >= 1).toBe(true);
    });

We tweaked the description wording slightly for the second test to specifically reflect the behavior we really wanted to see. Because a list is returned, it is a JSON object. We looked at example tests that showed how to return JSON body and formatted to fit our testing purpose. We are testing this scenario to see if an existing country returns a list of all cities associated. To really make sure the test is working the way it is intended, we used Mars, a non-existent country, to ensure that the returned list is empty. The point for this is so that we don't have the case of always returning true scenarios, ex) set x = true and it'll always return true.

test('should respond with 404 for invalid country', async () => {
        const response = await request(app).get('/Venus');
        expect(response.statusCode).toBe(404);
    });

We also needed to test for the scenario when the user input is an invalid country and return the 404 status error message. This test failed because we haven't gotten to our TODOs of checking i a country actually exists. When we added the filters, we weren't able to differentiate between an actual vs non-existent country. A new country query was required to validate the input. Placement of the query mattered as we wanted to achieve max. efficiency with the min. effort in order to boost performance. We had to modify our existing app.get('/:country')function to include another layer of error handling. Problems arose when we tried to return out of each local else if statements from each error handling; how do we return out of the mainapp.get function at the onset of initial error detection? Furthermore, what if no errors were detected and we needed the code to continue? We had to do a nested function for the very last elsestatement whereby nesting the entire city_query into it. We obviously wanted to avoid this nesting function situation but functionally speaking, the test passed. After asking on Slack, the concept of Promise/async await came up frequently. This is an area that I will have to investigate with Bill further. The placement of the return from app.get(':country') is also

test('should respond with an empty list for invalid country', async () => {
        const response = await request(app).get('/Venus');
        expect(response.body.length === 0).toBe(true);
    }); 

We also needed to ensure that when an invalid country input is given, the result should return with an empty list of array.

test("should respond with a 'visited' property", async () => {
        const response = await request(app).get('/Italy');
        expect(typeof response.body.visited === 'boolean').toBe(true);
    });

We are writing this test because we want to know if a country has been visited or not. The info is currently stored in our SQLite DB, but it is not returned in the list as in the SELECT query, we only requested city & country columns to be included.

test("should respond with a 'would_visit' property", async () => {
        const response = await request(app).get('/Italy');
        expect(typeof response.body.would_visit === 'boolean').toBe(true);
    });
});

The aforementioned logic applies here: we want the would_visit info to be returned in the JSON body on the browswer when the user requests a particular country. This is currently not the case. My subsequent challenges, besides learning aboutPromise/ async await, are:

berryjen commented 2 years ago

Upon seeking clarification on the topic of promise,async/await, I have discovered that it is much of a rabbit-hole. I do now vaguely understand the concept.Promise is a function. It is a token that represents work being done by some other process, in this case my SQL API. Thepromise token lets you schedule some further work to be done in your code, once the other process has finished its job. Much like the dry cleaner analaogy: cleaner gives you the ticket, and you come back in a few days. When you do pick up your clothes, you give the ticket and the cleaner may tell you it's either finished(success), still being cleaned(pending) or ooops! the machine went wrong and your clothes are now damaged(failure). If you happen to be busy, you can give the ticket to a friend and have the friend pick up on your behalf. You can give instructions where If clothes are ready, pick them up and drop them off at my place(upon success), or if clothes are messed up, file a complaint(upon failure). Thus, the work being done by the API is the dry cleaner, the ticket = promise object, the clothes at your house is the.then() action of the promise, and the file-a-complaint instruction is the .catch() action of thepromise. Promise is the syntactic sugar for async/await. Async/await is the syntatic sugar for function callbacks. It is a keyword that invokes a promise. They always go in pair and async always precedes await. Await undo thepromise and returns the value set within the promise. My other friend was kind to help me unwrap the nested function mess into its own individual functions. We decided to set the nested function to equal to a new function called abc(will change name later). Within abc, we summarized abc into 3 main functionalities of what it does, essentially the nested functions, in chronological order:

async function abc(req, res) {
    //validates country properties to be boolean
    validate_country_filters(req);
    //function will check if user country input exists against database
    validate_country_input(req);
    //retrieve an array of cities from database
    retrieve_array_cities(req, res);
}

This tidied up the code. The function needed to take on 2 params of req & res as it need user input (req) and return the reponse on the browser when it is called by app.get('/:country, abc). For the functions validating country properties, it only needs the reqparameter (user input, for example: Spain) and return the boolean values of visited &would_visitproperties. We then placed the old functions under the appropriate "group" of new function. Troubles arose when we couldn't treat undefined as an error in our original validate_boolean function and couldn't pass data between functions. We fixed it by applying the concept of separation of concerns. The original validate_boolean function was trying to validate boolean values and catching undefined (returning error) all in 1 go. The new validate_boolean are as follows:

function isValidBoolean(value) {
    if (value === true || value === false) {
        return true;
    } else {
        return false;
    }
}

function validateBoolean(value) {
    let isValidBoolean_result = isValidBoolean(value);
    if (isValidBoolean_result !== true) {
        throw new SyntaxError(`Value given is not a valid Boolean: ${value}`);
    }
}

validateBolean(value)does only the error handling of anything undefined, where as isValidBooleanonly handles T/F.

The next challenge is to figure out exactly what Promise does.

berryjen commented 2 years ago

We dived into promises & used thenew Promise, .then, .catch(next) to create a new

app.get'/v2/:country', (req, res, next) => {
validate(req)
        .then((req) => {
            console.log('database lookup');
            return { country: 'Croatia', cities: [ 'Rovinj', 'Porec' ] };
        })
        .then((data) => {
            console.log('return response');
            res.status(200).json(data);
        })
        .catch(next);
});

where it takes 3 paramsof req, res & next. Next is invoked only when an error is encountered and instructs the subsequent steps in the event of catching an error. Next is a function that JS provides its users with. Validate is a new function that takes in a request and gives a new promise that will performs tasks at a later time. When the task is successful, the first.thenis invoked with req as a param, which will look into our database and retrieve data; the second .thenis invoked , takes data as param, returns the data from our database injsonformat along with status code.

function validate(req) {
    return new Promise((resolve, reject) => {
        console.log('validate request');
        if (req.query.visited !== undefined) {
            validateBoolean(req.query.visited);
        }
        resolve(true);
    });
}

Thevalidate function takes in the req param and returns a new Promise in which it will either resolve or reject the task given to it, which is validating Boolean. However, we had to be mindful of the type of request given compared to the Boolean functions. For instance, return a list of all cities in Croatia is a valid question, but it is not a valid Booleanquestion as it doesn't yield in T/F. We had to modify the T/F values to be of string type as the user request is given in stringtype. Because the isValidBoolean only validates Boolean values of T/F (T/F are both valid Boolean values) anything undefined will yield in error. So our logic then became : "if visited is not undefined (as defined isn't a keyword) and it is aBooleanvalue, don't throw an error; if it is undefined & is not a T/F value, throw an error". When this task is resolved, then computer needs to know that the task has been successfully completed via using the resolve function. The subsequent challenges are:

berryjen commented 2 years ago

We tidied up our working code database as we have figured out a Promise model that solves our nested function problems, removing redundant functions. When I attempted to incorporate the already existing city_query and error handling message, the request on Postman was never resolved and db.all.... .then was undefined, along with the error that the database didn't know which table (country orcity) to retrieve the rows from. I was aware that I needed to define visited_city_query & would_visit_city_query variables before it can be used in the city_query itself, which was what I did. We had to change the variable names to match what was used in city_query , and specified cities.would_visit in the queries. Next on the list of bugs to fix was the error handling. In the original code, it was returning error inside the city_query before the .catch block. We separated the err, & rows where the err is thrown and then return only the rows. Also, I had mistakenly placed another throw error inside the .catch(next) as my logic at the time was that because .catchhandles the error, that is where you would place the error handling. Turned out that Throw & Catchis a matching pair where anytime an error is thrown, there needs to be a catch. Therefore, I would've needed to write another .catch somewhere in the code to catch the new error.

app.get('/v2/:country', (req, res, next) => {
    validate(req)
        .then((req) => {
            console.log(req);
            var would_visit_city_query = 'NULL OR cities.would_visit IS NOT NULL';
            if (req.query.would_visit === 'true') {
                would_visit_city_query = 'true';
            } else if (req.query.would_visit === 'false') {
                would_visit_city_query = 'false';
            }
            var visited_city_query = 'NOT NULL';
            if (req.query.visited === 'true') {
                visited_city_query = 'true';
            } else if (req.query.visited === 'false') {
                visited_city_query = 'false';
            }

            city_query =
                'SELECT city, country FROM cities JOIN countries ON cities.country_id = countries.id WHERE country = ? AND cities.visited IS ' +
                visited_city_query +
                ' AND ( cities.would_visit IS ' +
                would_visit_city_query +
                ')';
            console.log(city_query);
            db.all(city_query, req.params.country, (err, rows) => {
                if (err) {
                    throw err;
                } else {
                    return rows;
                }
            });
        })
        .then((data) => {
            console.log(data);
            res.status(200).json(data);
        })
        .catch(next);
});

Return rows & .then(data), the rows & data are different parameters pointing to an object that is a replica of the JSON in memory. This led to an insightful discussion regarding how memory works in JS. The rows are retrieved from SQL, stored in memory where as (data) is passed to the client inJSON format. .then(data) is the location where the copying of the JSON obj in memory occurs. We covered the differences between pass by value vs. pass by reference. The former being the physical content/value is passed whereas in the latter, the label/address is passed. In both scenarios, the physical locations are never copied. We realized that when an object is copied, its data is not overwritten. However, when a primitive type (string, Boolean, number etc), data does not get overwritten. This is because objects can contain different property types, therefore you need to be specific about which property is being modified. When there's only 1 data type to work with, that is the only thing one can modify. We have utilized the debugger as a tool to match my logic to that of the computers. It trains my brain to see what & where each line of the code does & goes. After our bug fixes, thereturn rows is still not passing the data back to the 2nd .then(data) Promise function chain. Instead it is only returning out of the callback function (err, rows). The next challenge is to figure out a way to incorporate thePromise chain into the db.all function.

berryjen commented 2 years ago

We wrote a new function as below that incorporated Promise chains into db.all functions. This particular function only handles the return of query search.

query_db(query, country) {
    console.log('query_db');
    return new Promise((resolve) => {
        db.all(query, country, (err, rows) => {
            if (err) {
                throw err;
            } else {
                resolve(rows);
            }
        });
    });
}

wheredb.all does work involving:

berryjen commented 2 years ago

I took a slight detour and started a mini music-API project to consolidate my knowledge up to this point. Once that is completed, we will resume the learning on travel API with getting the remaining tests passed, then moving onto data update, cloud hosting & multi-user.

berryjen commented 1 year ago

As we were attempting to make make the test "should respond with a 'would_visit' property" for ('/v2/Italy') pass, we realized how much work it. was to pass the data we wanted among all the functions. We started off by hard-coding the JSON object (end result; with would_visit & visited properties) we wanted within the .then Promise chains of app.get('/v2/:country), which meant that we also had to change the variable that contained country initially, its method of storing and accessing the content. Tracing back through the tree of functions proved to be challenging, and was a sign that we needed to refactor our code according to separation of concerns where 1 function only handles db queries, another that handles user requests and another that only retrieves countries /cities. We modified function get_country(req) and stored country row JSONinside a new var data as this allowed us to control exactly what went in the new object & isn't interfering with the property that might be included in the given req.

else {
                    var data = {
                        "country": row,
                        "req": req
                    }
                    resolve(data);
                }

This also meant that we had to change the parameter names passed for each of the functions that required the use of var data, which included then second .then(data) from app.get('/v2/:country'), as well as modifying how var datawas to be retrieved ; from req.query into data.req and stored said value into var req as nowreq is a property of data.

We then discussed how we would logically separate each concerns into 1 distinct function by using pseudo-code where taking in 1 parameter was only requirement to execute the function. In this scenario, the function communicates to our db returns the details of a specific country by name. This function doesn't need to know where the namecame from(API, own db), or what we'll do with the result. Noreq or resneeded here. When defining the paths (routes) that our API will respond to we can keep the code more readable, by mapping paths to functions that know how to respond to user requests. This is improves readability because we have simplified the paths our API has available and identify the functions that handle their requests. As db queriesare no longer part of the route, understanding exactly the content that is being handled becomes extremely clear.

This is an example of how we might handle a user request for a country, e.g./api/italy

app.get("/api/", handle_get_countries);
app.get("/api/:country", handle_get_country_by_name);
app.get("/api/:country/:city", handle_get_city_by_name); 

We extract the country name we are looking for from the request; the userId from theheaders (mock data). There is now enough info to call the getCountryByName() function, which then responds to the request. There is no SQL queries here. However, in this mock scenario we haven't shown Promises or error handling.

const handle_get_country_by_name(req, res){
    var country_name = req.params.country_name;
    var user = req.header.userId;
    var country = db.getCountryByName(country_name, user);
    res.json(country);
}

While I was attempting to re-write the tests, I came across the challenge of deciding which property hierarchy to include. I'd get confused which reference point to continue testing. As Bill pointed out, it is the returned JSON object that we are testing against, not the code. expect(response.body.data.country.name).toHaveProperty('country.visited') was erroneous in that I thought as we had previously modified the code to storecountry (as rows) inside var data; therefore, to retrieve the country, I had to go down the var data hierarchy. However, var data &response.body are 2 unrelated properties and mixing them together gave me the error of either can't read property of data/country/name of undefined. expect(response.body).toHaveProperty('visited'); was the correct logic as the country is returned in response.body and you wanted to test if that body contained a visited property.

While we were checking app.patch to make sure the updates were successfully accommodated, we observed a difference in behavior among the terminal, server & Postman. On Postman, the updates took effect immediately. However, on the server, the changes weren't registered. On the terminal, the queries run vs what the Boolean values returned weren't the same. Turned out that we had been comparing the string value of Boolean instead of the actual Boolean values themselves. We created a new function that specifically checked the string Boolean, and this solved the debugging issue.

function isValidBooleanString(value) {
    if (value === 'true' || value === 'false') {
        return true;
    } else {
        return false;
    }
}

The next feature to be added is multi-user, & ORM. We have come across a few suggestions from tech forums to research into ORM (object relational management) to simplify writing queries, as to avoid embedding SQL queries within the routes. To incorporate multi-user, we will have to re-structure our data that will track users & map their data according to user ID.
We briefly discussed how we might use pseudo code to help us define the function as follows:

const db;
db.getCountryByName(name, user) {
    const query = "SELECT country FROM countries WHERE name = ? AND user = ?";
    const country = db.get(query, name, user);
    return country;
}
berryjen commented 1 year ago

After our research & consulting articles regarding pros & cons of ORM, we have decided to opt for query builder Knex to help us write simpler queries. Pros:

Cons:

Based on the cons, we made the choice to forgo ORM and proceede with Knex, as its syntax is very similar to SQL, and it solves all of our current pain-points, and allows us to incorporate multi-user queries more seamlessly.

We created a brand new project with Controllers,Model , Database & Migrations.

In database, that is where we decide which database to use- production or development- to ensure that data remains integral when structural & code changes have taken place. This brought us to the discussion of the differences between each named environments. Each database contained within each environment are all separate and does not know the existence of the other db. The naming of each environment is independent to the activities that take place; however, the same activities can occur in each environment in theory.

Migrations is separate from database. They apply to all things data structure related modifications only (skeleton) and tracks the incremental changes on an existing db.

Controllers is where the connection between model& view is made. However, for this project, we use res.json as the view. It's an object available already in Express and it contains the array of objects that users needs. It's the most efficient way to return content to the frontend.

Lastly, in models, this is where all the queries are written.

The next task is to create a countries.js file under models that offers various filtering feature:

For the time being, we have not committed our code to my GH repo as of yet because we are trying to simulate the commercial workflow of a "production" environment (where the app "goes live") which will be on Bill's local machine. The repo on my local machine will be purely "development" environment (where all modifications take place). Once Bill has had the chance to set up "production", then the push will happen.

berryjen commented 1 year ago

Upon attempting to create the new visits table independently, I ran into a few issues. To begin with, I wasn't aware that duplicated copies of

@param { import("knex").Knex } knex
 * @returns { Promise<void> }

and exports.up & exports.down would break the code and instead had to combine adding & deleting the tables within the same exports.up & exports.down, with one

@param { import("knex").Knex } knex
 * @returns { Promise<void> }

preceding each of of the table methods. I've also learned that you need to run migrate:up & migrate:down in pairs to see exactly the modifications that take place. When I ran the command migrate:up, the error message already up to date was displayed, which is a clue that migrate:down needed to be also run to undo the previous migration before you run migrate:upagain for new changes to take effect. Data type for each column has also been modified to better reflect the purpose: integer for country_id , user_id, & visits. The visits table still wasn't added successfully despite all the debugging, so I tried a different command pair of migrate:rollback & latest. The difference between up & latest, down& rollbackis that the changes are made all at once in the same batch vs individually. This didn't solve the problem either, so Bill suggested to completely delete the database file devsqlite3 to start from scratch. This debugging process has taught me to keep migrations small & simple, in addition to never change them once they work. Instead of modifying a migration, adding new migration files for future modifications to save all of this mess. This was a new learning curve for me as way back when I was writing queries via SQL, there wasn't a method to automatically apply the modifications (equivalent of migrate:up & down)to the db. There was a SQLfile that we had to keep commenting out some queries, or picking and choosing the ones we wanted to run by using command P. I was confused as to why you'd want to undo the changes that you do intend on making and then having to delete & redo the changes again; this seemed counter-intuitive. As Bill kindly explained, this is needed to avoid unintended consequences during deployment in the production environment. It also confirms that you see exactly all the methods work as intended: expectations vs reality. This makes the programmer absolutely confident in the code that it does what it's supposed to do. After the visits table has finally been successfully created, new visits files have been added under controller, models & route. Bill has set up a production environment on his GitHub that provides opportunities to practice working in "sprints" / "tickets" and open-sourced contributions that involves the cycle of

Automated tests have been written in production to ensure that branches can be merged. An important note is to always run tests before pushing the commit. My next issue to fix is to write tests for the visitspath under #travel-log #11

berryjen commented 1 year ago

I successfully and independently provided test suite for /visits that covered the following 4 scenarios:

 expect(res.statusCode).toEqual(200);
    expect(res.body[0]).toHaveProperty('country_id');
    expect(res.body[0]).toHaveProperty('user_id');
    expect(res.body.id).not.toBe(null);

the indexing is required as JS needs to know which item in the array of json objects the test is specifically testing for.

Upon attempting to implement issue #12 /visits/:id on my own, there was 1 major bug I ran into while sorting out a standardized date. When I was testing paths for /visits, all the data was returned; however, /visits/:id had an error message of visit.arrival_time.toISOString is not a function which was when we went down the rabbit hole of the difference between Date() & new Date(). Initially, we thought the bug was simply removing '' in our SQL query of

INSERT INTO  "visits" ("user_id", "country_id", "arrival_time", "departure_time") VALUES ('1', '1', 'CURRENT_TIMESTAMP,' 'CURRENT_TIMESTAMP');
where it stores the string CURRENT_TIMESTAMP as a string
to 
INSERT INTO  "visits" ("user_id", "country_id", "arrival_time", "departure_time") VALUES ('1', '1', CURRENT_TIMESTAMP, CURRENT_TIMESTAMP); 

where it stores current date and time as a string. However, what was returned from our SQL select query was a string, instead of a date object, as everything is stored as a string in our DB. Our challenge was to know what format to convert it to when it is read out. We have logged and tried to compare the difference between what we get back from the DB (strings) and what we have after we try and parse that into a date (hopefully dates). Turned out thatDate() will only returns the current time at the instance of typing whereas new Date() can return any date you set it to be (going forward or back in time). We had to use

const at = new Date(atTs);
 const dt = new Date(dtTs);

as we have decided to use current time to simplify our data for the purpose of just populating some data for testing. Which means the current time stored in DB will be in the past by the time it is read and toISOString() only works with new Date() and not Date(). It is crucial to remember that you have to be actively thinking about when the time is current, future, or past and in which environment that takes place. In conclusion: In the DB we only store one format… we need to be strict about this. In the APIwe should accept and return one format for consistency… we should be strict about this. In the front end we should be super flexible and convert to the format the APIexpects. We should expect it to validate the format we send and give us an error if it isn’t right. We had to convert the date back to a string in order to be printable on the console, as it only takes string objects.

// Parse dates in the DB from strings to number (seconds since UNIX epoch)
  const atTs = Date.parse(visit.arrival_time);
  const dtTs = Date.parse(visit.departure_time);

  // Conver these numbers to dates
  const at = new Date(atTs);
  const dt = new Date(dtTs);

  // we can now log these
  console.log('arrival_time', typeof at, at.toISOString());
  console.log('departure_time', typeof dt, dt.toISOString());

  visit.arrival_time = at.toISOString();
  visit.departure_time = dt.toISOString();
  return visit;

I've also realized that In table plus it is the development database of ./dev.sqlite3 that I have tested onlocalhost. In production the data is what real users of your API would have created, i.e. not just my test data. So when Bill added some production (aka "real" user data), the data displayed differed from what I saw returned onlcoalhost, which originated from my development environment.

The final issue I will resolve is #15, where a new feature that allows users to submit a POST request to visits in order to create an entry in the visits table.

berryjen commented 1 year ago

In our attempt trying to write error handling code, we first had to create a new class error that will separate the errors into more specific categories instead of just a 'one size fits all' catch.

class ConstraintIdNullError extends Error {
  constructor(message) {
    super(message);
    this.name = 'ConstraintIdNullError';
  }
}
if (err.code === 'SQLITE_CONSTRAINT') {
      throw new ConstraintIdNullError(`visit can't be created due to null user or country ID; userId='${userId}', countryId='${countryId}`);
    } else {
      console.log('unable to create visit, SQLite error:', err);
      throw new Error('unable to create visit');
    }
  }
};

If the error is due to a SQLITE CONSTRAINT, where country id and/or user id are null, then send the tailored message. For everything else, throw the generic error of unable to create a visit.

We also revisited and re-dissected the subtle differences between Promises, async & await as a result of writing the error handling block, the difference being the value needs to be returned first before work can be continued. Async & await is a layer on top of Promises that makes it easier to write tests & codes. It's a 'new & improved' version of Promises. Promise chains, instead, hand a token in the meanwhile while the work is being done.

Discrepancies were observed from the time that a user submitted vs. what was returned in Postman. It is important to note that setting the time in DB to be the same for consistency, and Zulu is the industry standard. Some DB types will allow constraints to be directly placed on DB itself, but some won't. Therefore, the responsibility lies within the code validation in the development environment. To investigate the discrepancies further, we created 2 separate test suite where one targeted the testing of standardized Zulu time format, and the other specifying timezone. Previously, based on the code written, an automatic conversion took place somewhere as per observed when we experimented with the following: non-Zulu, but time-zone specified, human date(June 8, 2022), gibberish date (a string of text) and finally, the most confusing one, ambiguous date (1/3/2022).

describe('POST /visit', () => {
  const visit = {
    user_id: 1,
    country_id: 3,
    arrival_time: '2022-10-27T09:27:25.000Z',
    departure_time: '2022-10-26T09:27:25.000Z',
  };

  it('should respond with a new visit', async () => {
    const res = await request(app)
      .post('/visits')
      .send(visit);
    visit.id = res.body.id;
    expect(res.statusCode).toEqual(201);
    expect(typeof res.body.arrival_time).toEqual('string');
    expect(res.body).toHaveProperty('country_id');
    expect(res.body).toHaveProperty('departure_time');
    expect(res.body.user_id).toEqual(visit.user_id);
    expect(res.body.departure_time).toEqual(visit.departure_time);
  });

  it('should create the visit in the DB', async () => {
    const res = await request(app)
      .get(`/visits/${visit.id}`);
    expect(res.statusCode).toEqual(200);
    expect(res.body.user_id).toEqual(visit.user_id);
    expect(res.body).toHaveProperty('arrival_time');
    expect(res.body.arrival_time).toEqual(visit.arrival_time);
  });
});

we stored visit.id = res.body.id so that we can retrieve the info we created for the second sub-section of the tests. What we noticed was the following pattern: If non-Zulu time is inserted, but with timezone specified at the end, the conversion occurs here. Everything is converted to Zulu; however, if the formatting isn't specified in Zulu or timezone, it assumes the server's current location instead.

describe('POST /visit (with timezone)', () => {
  const visit = {
    user_id: 1,
    country_id: 3,
    arrival_time: '2022-10-27T09:27:25.000+0100',
    departure_time: '2022-10-26T09:27:25.000Z',
  };
  const expectedArrivalTime = '2022-10-27T08:27:25.000Z';

  it('should respond with a new visit', async () => {
    const res = await request(app)
      .post('/visits')
      .send(visit);
    visit.id = res.body.id;
    expect(res.statusCode).toEqual(201);
    expect(res.body.arrival_time).toEqual(expectedArrivalTime);
  });

  it('should create the visit in the DB', async () => {
    const res = await request(app)
      .get(`/visits/${visit.id}`);
    expect(res.statusCode).toEqual(200);
    expect(res.body).toHaveProperty('arrival_time');
    expect(res.body.arrival_time).toEqual(expectedArrivalTime);
  });
});

The placement of visit.id = res.body.id; was originally at the very end of block, which resulted it being never executed due to the fact that an expected test failed. The failing test is actually a separation of concerns, where we purposefully introduced a wrong time format for testing, in which only 2 tests were expected to fail. Instead, 3 were observed. Visit.id was never captured due to the fact A TEST had failed, and it happened to coincide with the wrong date format.

The next issue to tackle is to use Knex joins to display country names onto the JSON obj instead of country_id, as this new feature will be more helpful for users to refer to.

berryjen commented 1 year ago

Upon consulting the Knex documentation, there is a bit of a variation in the syntax in how the queries are structured. It reads slightly closer to a function then query statements. There are methods that took in arguments that specified the tables to be joined, and columns to be returned.

exports.get_all = async () => {
  const visits = await db('visits')
    .join('countries', 'visits.country_id', '=', 'countries.id')
    .select(['visits.id', 'user_id', 'country_id', 'name']);
  const visits2 = {'id': visits[0].id};
  console.log(visits);
  return visits;
};

The returningjsonobject resulted as follows:

{
  "id": 1,
  "user_id": 1,
  "country_id": 1,
  "name": Canada
}

But we want to manipulate it so it returns the following instead as it will be deemed more useful:

{
  "id": 1,
  "user": {
    "id": 1
  },
  "country": {
    "id": 1,
    "name": "Canada"
  }
}

To achieve this, we had to do a separate exercise just to practice with data manipulation. In order to create nested objects within theuser & country, we had to store the object returned from the newly combined tables into a new constant that held it. Then, we created another empty new array that would hold the modified & desired version of the nested objectwithin the object. To display all the visits data, a method that would iterate each item within the array is required that will also take in a function that accepts the stored data as an argument. A brand new constant had to be created inside the function to store the newly formatted json object with the nested structure. To add filtering feature (as we wanted not to return a user_idwith text strings), I coded an ifstatement to support it. Finally, we had to insert the new constant to the new arrayto complete the object manipulation. c``` onst new_data= [ ]; data.forEach(function(properties) { const v= { 'id': properties.id, 'user': {'id': properties.user_id}, 'country': {'id': properties.country_id, 'name': properties.name}, };

if (typeof properties.user_id !== 'number') { return }; new_data.push(v); });

berryjen commented 1 year ago

After practising JSON object manipulation exercises, Bill and I compared solutions to discuss the various approaches to solving identical problem set. Part 1 of the exercise, my original code returned an array containing a property of actors object, which holds an array of actor names. This wasn't quite the object structure as what Bill had, so we probed further. Turned out that some of the {} &[] brackets blended in with my email background setting, thus I was unable to properly view the desired data structure.

const actors = []; 
data.forEach((d) => {
      const a = {
         actors: d.actors,
      };
      actors.push(a);
});

vs. the desired final data structure as follows:

const actors = [];
data.forEach((d) => {
  (d.actors).forEach((actorName) => {
    actors.push(actorName);
  });
});

Where an array containing strings of actor names were returned. Bill had pointed out a new syntax to avoid the nested function...(), called aspread. In order for this method to work, the following 2 conditions need to render true: a) the items have to be iterable ; b) the "place" the items are being spread into accepts more than 1 argument. This approach could either be considered part of refactoring depending on the context of the original code written. The spread method required an extensive JS knowledge that may not be applicable to junior engineers; however it is aesthetically more concise. Readability vs. refactoring depend on the purpose: is it to provide better clarity by being explicit, or is it to be succinct with fewer lines of code?

In the second exercise, we both approached the problem the same way.

const list = [];
data.forEach((d) => {
  const sml = {
    title: d.title,
    released: d.releaseDate,
  };
  list.push(sml);
});

The third exercise required identical data structure as the first challenge. This meant that I had to do another nested forEach function in order to iterate through the data object & the actor names. However, this time, the task was to remove all duplicated actor names from the entire move list. I accomplished this by adding an if condition stating that if the actor name is not included in the list, the push it to the empty array.

const list2 = [];
data.forEach((d) => {
  (d.actors).forEach((actorName) => {
    if (!list2.includes(actorName)) {
      list2.push(actorName);
    }
  });
});

The last challenge will require me to return only 1 genre object with a list of movies belonging to the object. Each movie can appear several times under different genres; however, each genre can only appear once (unique).

berryjen commented 1 year ago

We tried approaching the last challenge via "remove duplicates" method via using nested forEach loops but soon encountered the issue where 2 objects with identical properties & values are not indicative of the same objects. The logic is inherently flawed based on duplicates. On top of that, nested forEach methods can become complicated to unravel the deeper one goes in the code. Therefore, the utilization of new Map() to completely overhaul the existing data structure into one that allows a unique key (genre) & values(movie titles) pair.

const genresList = new Map();
data.forEach((d) => {
  (d.genres).forEach((genreName) => {
    if (genresList.get(genreName) === undefined) {
      genresList.set(genreName, [])
    }
    genresList.get(genreName).push(d.title);
  });
});
console.log(genresList);

We needed to create a new const that stored the Map object. We then ran forEach method on the movies dataset that took in a callback function of an IF statement where an empty "bucket" of genre array is created before anything can be pushed. We then retrieved the parameter genreName from the array genreList and pushed all the movie titles associated with each genre to the genreName array. The returned object was the following:

Map(8) {
  'Crime' => [
    'Nyckeln till frihet',
    'Gudfadern',
    'Gudfadern del II',
    'The Dark Knight',
    '12 edsvurna män',
    'Pulp Fiction'
  ],
  'Drama' => [
    'Nyckeln till frihet',
    'Gudfadern',
    'Gudfadern del II',
    'The Dark Knight',
    '12 edsvurna män',
    "Schindler's List",
    'Pulp Fiction',
    'Sagan om konungens återkomst'
  ],
  'Action' => [ 'The Dark Knight' ],
  'Biography' => [ "Schindler's List" ],
  'History' => [ "Schindler's List" ],
  'Adventure' => [ 'Sagan om konungens återkomst' ],
  'Fantasy' => [ 'Sagan om konungens återkomst' ],
  'Western' => [ 'Den gode, den onde, den fule' ]
}

Which gave us the right mapping but an object structure instead of the the array.

const arr = [];
genresList.forEach((m, g) => arr.push({genre: g, movies: m}));
console.log(arr);

To convert the object into an array, another new const was created to store the new array. Next, a forEachloop is placed on the existing genresList map object that took a callback push function that appends the key(genre) value (movies) pairs into the empty const array.

berryjen commented 1 year ago

Upon attempting to merge my working branch into the project's main branch, it was successful. I quickly realized that this was because we had made changes to our original code to reflect the desired data structure, which is an array of nested objects within objects. To resolve this particular issue, I had to change the code base for the tests as the original data structure no longer exists.

describe('GET /visits', () => {
  it('should respond with status 200 ok', async () => {
    const res = await request(app).get('/visits');
    expect(res.statusCode).toEqual(200);
    expect(res.body[0]).toHaveProperty('country');
    expect(res.body[0]).toHaveProperty('user');

As both country_id &user_id are no longer valid properties, it had been changed to country & user respectively.

The next 2 tasks for me is to include more tests that will test for properties of the country object, and to data manipulate the object returned from models/visits/get_by_id to be identical as models/visits/get_all on the browser.

berryjen commented 1 year ago

To further improve upon user support, we have taken action to display data that correspond with your own visits. We began by ensuring all our model methods take userID as a parameter and only return results for that user. The user_id param goes under visits model as it makes logical sense to return all visits related to that user only. The routes (the path we take for the object to be returned) remain unchanged because we've hard-coded the user_id in the controller, thus the API is aware which user to return visitsfrom (filer feature). When we attempted to include user_id param within the path get_by_id, we encountered 2 scenarios where where the visits obj can come back as undefined (object is non-existent):

Logically, the next step would be to create a new migrationstable, named tokens with 3 columns:

berryjen commented 1 year ago

When I attempted to create a new migration for the tokens table, I ran into an error while trying to import it into Tableplus. The Knex code was working fine, the table structure itself (rows, columns) was not returned on Tableplus, instead, it dumped all of the VSC code. I ran thenpx Knexcommands correctly, so I knew that wasn't the bug. Turned out that I had incorrectly named the file without running the Knex command (npx knex migrate: make). After the new migration was finally successfully imported, we then had to make a decision on which model to place the token function in, or to create a brand new tokens model connection. We reasoned that with authentication, the first step is to give a token, validate it against db, if it exists, return a user. Because the function is returning a user, it makes logical sense to group it within the users model.

exports.get_by_token = async (token) => {
  const user = await db('users', 'tokens')
    .join('tokens', 'tokens.user_id', '=', 'users.id')
    .where({ 'tokens.bearer_token': token })
    .first(['users.id', 'users.name']);
  return user;
};

There was no need to include error handling in this function as the code does exactly what it's supposed to do. Since it is only returning an object, the error logic isn't required here. The function that calls get_by_token will handle the error (either generic or specific).

When I attempted to create a brand new token routes, controllers & models file, the object was returned in the browser despite the fact that I had fine-combed through all the codes (caught 2 bugs: spelling mistake on json and forgetting the codemodule.exports = router; in the tokens routes file. The same error 404 not found kept displaying. Bill hinted that it if all the codes were perfect amongst the 3 files, might be something broader that is causing the something doesn't exist error. Turned out that I had forgotten to set the link in the gateway file app.js, where connections are made to the router interface. It sets up a "talking point", allowing the browser to know which path of / is called as there are 4 live paths all starting with / . const tokensRouter = require('./routes/tokens'); app.use('/tokens', tokensRouter);

The next sprint is to write some updated tests for users.test.js and some new ones for tokens.test.js

berryjen commented 1 year ago

We have created 2 new tokens functions where one retrieves a token via userId and the other userId via models token.

exports.get_token_by_user_id = async (userId) => {
  console.log(userId);
  const token = await db('tokens')
    .where({ 'tokens.user_id': userId })
    .first(['user_id', 'bearer_token']);
  return token;
};
exports.get_user_by_token = async (token) => {
  console.log(token);
  const userId = await db('tokens')
    .where({ 'tokens.bearer_token': token })
    .first(['bearer_token', 'user_id']);
  // if (userId === undefined) {
  //   throw new NotFoundError('invalid token');
  // }
  return userId;
};

In order to implement the multi-user new feature, we have decided to use Bearer Passport as the middleware to authenticate users given a token. We then proceeded too wire Bearer Passport in app.js, as that is the first authentication point of entry for users when they initially open the app. Next, we enabled the middleware for tokens routes so that it routes authentication from app.js to routes tokens.js router.get('/:user_id', passport.authenticate('bearer', { session: false }), tokensController.get); Error handling also had to be considered for invalid tokens, so we used a similar error handling class from models visits.js as follows:

class NotFoundError extends Error {
  constructor(message) {
    super(message);
    this.status = 404;
    this.name = 'NotFoundError';
  }
}

We attempted to change the tokens controller but later discovered that it was unnecessary due to Passport bypassing the controller itself, where it "liaises" between Express server & routes. It was also important to keep the async/ await structure consistent as the SQLite database works on that premise, otherwise the return we'd get is an unresolved Promise implicitly. I will need to further clarify the general error handling class as to whether to separate it into 2 individual categories: one error for invalid user, and the other for invalid token.

berryjen commented 1 year ago

Bearer Passport has now been successfully wired to the back end, which does the token authentication. This is step 1 of the multi-user feature implementation, where token verification. Our next step would be to implement authorization, which then decides what the user is allowed to do after being verified. The route logic occurs at this phase, which verifies the user. When token is provided, Passport middle ware is authenticated, then moves on to token.get in controllers Session was set to default false passport.authenticate('bearer', { session: false }), but it depends on the situation. Sometimes it is better to use stateless (session-less) authentication (json web token) so that database storage isn't involved for tokens.

berryjen commented 1 year ago

I realized there was an inherent logic error app.js. Instead of if (token === undefined) {}, it should be if (user === undefined) {} because we are checking if the user is valid, not if the tokenis valid. Any string token given by any user (even if it's gibberish value) is a defined token, thus the onus lies on checking if said token matches a user in db. If user exists, then the token is valid. I have wired up bearer', { session: false }) to visits.js routes under both .get paths of '/' & '/id', token.js routes under both .get '/' & '/:user_id', and lastlyuser.js routes under.get'/:id'. Because Passport is now being implemented on these routes, in order to retrieve the JSON in visits(info about arrival & departure time, country_id etc), we had to reference req.authInfo.user_id, instead of hard-coding the user. Authinfo is an entity created by Bearer Strategy as part of the authentication process My questions now are: a) does Passport need to be implemented onusers.js`` routes forget'/'; meaning, does it make still make sense to allow users to see the credentials of other users (name, user_id), or does it make sense to allow users to see only part of the credentials (names only) b) does it still make sense to have.get'/' path for visits.js routes as users now have to authenticate themselves? Subsequently, I need to wire up bearer', { session: false }) for visits.js routes but under /post and find out where user_id comes from as it might not be under {req.authInfo}.

berryjen commented 1 year ago

Upon further investigation on visits controller, i was unsure where the user_id would be retrieved from under POSTrequest. Previously with GET request, user_id was wrapped in Authinfo object as it was something that Bearer strategy. Instead of retrievinguser_id regularly within the body (this can lead to phishing), req.Authinfo.user_id can be passed as the param and retrieve it the same way as well. I tried to validate the existing authentication logic by supplying the param with a valid token for user_id1, but in the POST body i tried to imposter a user that is yet to exist in the database, user_id 3, and created a false visit as user 3. The POST req was accepted, but inside the response body, the user_id was defaulted to user_id 1, and thevisit_id jumped up by 1 number. I thought authentication was working like it was intended to because the logic had recognized that. However, the POST req was only partially successful as the instructions(body) didn't match the result(response), which was a very nuanced difference compared to what I understood. What I hadn't realized was that the visit id did not auto-increment as it should because I had unintentionally pre-setted it. As a user, you won't know how many visitshave been created in total in the db and the fact that the request was successful was the second clue.

if (req.body.id !== undefined) {
      return res.status(400).send('Bad Reqest, should not include id');
    }
if (req.body.user_id !== req.authInfo.user_id) {
      return res.status(401).send('Unauthorized, user_id does not match token');
    }

The new IF statements take care of the error handling for the edge cases described above. I've also discovered the nuanced difference between 401 Unauthorized vs 403 Forbidden- the former is case-by-case ex) user 1 is allowed to view/ perform certain action but user 2 is not; the latter deals more with a blanket statement- the individual has not been granted permission to do the task ex) not an admin. res.sendStatus(404) only sends the generic, predetermined error message, where as res.status(401).send allows you to include a customized error message, telling users specifically what is wrong. It has been pointed out to me that because nothing is being done to the response body other than it being a record in db (response not passed onward etc), is the IF statement

if (req.body.user_id !== req.authInfo.user_id) {
      return res.status(401).send('Unauthorized, user_id does not match token');
    }

is still necessary? If that's the case, then the request would still be partially successful....Should we introduce admin members (they'd have the ability to create POST requests on behalf of others) then we can add in another IF logic to accommodate that scenario.

berryjen commented 1 year ago

Now that Bearer Strategy have been implemented in most of the routes, it was time to get the tests passing again accordingly. We fixed the tokens unit tests first as practice because I was puzzled where to place the path to be tested and it turned out to be quite obvious: const res = await request(app).get('insert test path'). To be thorough, it is always necessary to test both the positive and negative scenarios, and in this case, status 200 & 401. We have created a new customized error in app.js

const user = await tokensModels.get_user_by_token(token);
  if (user === undefined) {
    console.log('invalid id', user);
    const err = new Error('Inavlid token');
    err.status = 401;
    return done(err);
  }

instead of the generic 500 because this test case specifically dealt with an invalid token and the logic resided here because authentication is the first step that needed to occur when a user logs on. In trying to create a mental model of understanding of errors, it is easy to oversimplify concepts in attempt to relate them to similar-sounding keywords to compartmentalize them. Throw- the analogy would be tossing the error in the air with no control over who/what catches it Call- appoints something else to pass the object to. Relates to functions Return (not the same as throw!) give it back to the owner Validation can be broken down to 2 parts depending on where the authentication & authorization checks occur: 1) checks to see if token is valid. This is the first half of the process which authenticates a user based on a given token. The code goes to the database & compares token provided to existing ones. 2) the second half, which is authorization, takes the authenticated user to database, returns a list of requests (visits/users/tokens etc). -> in this step, the logic gives you authorization to implement the request ie) if the info is retrieved, presented or not vs. the logic not bothered to retrieve the info due to invalid token. -> the act of displaying is the perceived outcome, not where authorization actually happens.

A real-life scenario regarding where authentication occurs that would have a major impact is credit score requests. If someone malicious is trying to lower another person's score, the following sequence illustrates how the request would be made: user req --> credit agency --> API credit db --> score formatted in JSON --> returned to user

If authentication & authorization occurs at the first step, the request won't be successful all together, no API call is made and no list will be retrieved. No lowered score. However, if authorization occurs later in the process, the API call will be made but list won't be shown due to invalid token, someone's score will be impacted due to the API request itself being successful, just the results not presented to the user who requested the info.

berryjen commented 1 year ago

Upon fixingvisits unit tests, they did not test as i anticipate. It was weird to me that that the error 500 was regarding Knex query when we haven’t touched db ormodelsrecently, so I knew that couldn’t be where the bug was. I also thought it was strange how it('should respond with status 401 with invalid token', async () => {}passed the test but status 200didn’t, when it was just the validity of the token that had changed- i quadruple checked the unit test and it looked fine to me(correct route & token), so i shifted my focus on routes& controllers files. I knew Bearer strategy was wired up successfully because authentication & authorization have been working, so that told me that something was missing in the controllers file. I honed in on exports.list& exports.get all in the meanwhile thinking that why did the test pass when i gave it an invalid token (401 unauthorized showed on both the terminal & browser) but when i gave it a valid token, the list ofvisits made by the user associated with the valid token was not returned. I was comparing between exports.list& exports.getand noticed that req.authInfo.user_id is the arguement that does the authentication… i typed in the address bar with visits?access_token=ABC123but the controller was not expecting that param cus I hadn’t given it. So while the logic could tell whether the token was valid or not, it wasn’t able to retrieve the list of visits. I tested out my hypothesis by pasting req.authInfo.user_id, ran the test, and the results confirmed my theory!

berryjen commented 1 year ago

Deep diving further into/visits unit test, specificallyGET /visits/13, I had difficulty debugging why 'it should respond with a single visit with valid token'did not pass but the opposite 'it should not respond with a single visit with invalid token'did, with identical behavior for both user 1 & 2. The error messages from both the browser & terminal were conflicting as well. The former showing that the request was successful with JSON object returned as I had expected, and the latter a 404 not found. When i poked around where404came from, it from visitsmodels. It turned out the bug lived inside the create-test-visitsseeds file, where test records are populated for testing purposes, used by all routes. I hadn't included a value for a specific visits_id to carry out testing, thus when GET /visits/13was run, it was using a randomized visits_idvalue as because such values are auto-generated as written in Knex. To ensure successful tests, you want a fixed test values (for all the properties) so a GET request can be made and fetched.

The last bug occurred when the test it 'should create the visit in the DB with valid token' failed within the /post test suite but passed within the /post (with time zone) test suite. It was puzzling to me at first because both individual tests were essentially performing the same purpose. What I hadn't realized was that in the previous single test it('should respond with a new visit with valid token', i had set visit.id = res.body.id; which meant that in theresponse body, the test was expecting a valid id value. When i had included visit.id = res.body.id;within the it('should not respond with a new visit with invalid token'test, it was expecting a valid id value from an otherwise empty body due to invalid token given, and this would lead to visit.id = undefined. Because the tests run sequentially in the order they were written, the value of visit_id= res.body.id; from the first test case was carried over to the second test, which tests for the invalid token, where visit_id = undefined. This value of undefined would then carried to the third test, which would make a /GET request for an undefined visit_id of the newly created visitin the test db, thus resulting in a fail of the test case. To debug this, removing visit_id= res.body.id; from the test it('should not respond with a new visit with invalid token') will make the test suite pass.