Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Resolve Broken Integration Tests #1

Open
wants to merge 12 commits into
base: master
from

Conversation

@cutandpastey
Copy link
Owner

commented Sep 9, 2019

Problem

We need to create a basic TCP server which provides a simple CRUD API, the following methods were implemented:

  • Create
  • Get (Read)
  • Insert (Update)
  • Format (Update)
  • Delete

Solution

I have setup a simple solution similar to a traditional MVC architecture:

Controller

Once a socket connection is established a controller instance is created. This controller maps its internal methods to commands that can be issued through the socket. Unknown methods will error.

Model

Ive setup a simple ContentModel that encapsulates inserting content into a note, adding formatting marks and retrieving either text or markdown content.

View

Currently there is no view component, models can return text or markdown content and the controller sends responses through the socket connection. This should be improved, please see the "Improvements" section of this PR for more details.

Store

Simple in-memory abstraction for persisting data.

Steps to Test

Integration tests have been implemented. This PR fixes all of them.

Improvements

  • Error cases from within the system should be handled correctly and 500's should be returned. Currently this is not catered for although a try/catch block is provided so this functionality can be implemented easily.
  • Generic functionality should be provided for each controller method, for example 404 and blank 200 responses are manually coded, this could be abstracted with decorators easily. An abstraction like this would provide a more modular solution decreasing development time when implementing new commands.
  • ContentModel's currently serialise their internal content to either text or markdown formats. This should be moved into a view construct allowing for application component specialisation and a more correct separation of concerns.
  • Currently the controller is responsible for managing the storage of models. This is done through the Store abstraction. This could be improved by moving the store abstraction into the Model and using a pattern similar to ActiveRecord. This would clean up the controller somewhat.
  • Storage is only held in memory. This could be improved by adding persistent storage like levelDB or Postgres or some such.

@cutandpastey cutandpastey changed the title Jp technical test Resolve Broken Integration Tests Sep 9, 2019

const args = chunks.splice(1).map(this.sanitise);
// Call the command with the args. This is really useful as it allows us
// to quickly iterate on our controller methods
this[cmd](...args);

This comment has been minimized.

Copy link
@Calyhre

Calyhre Sep 10, 2019

Isn't this a quite important security issue? You could send constructor, apply or call to this object and have a nice remote code execution for example

// splice is useful here because it mutates the args array
const content = args.splice(-1);
const note = store.get(id);
const [start] = args;

This comment has been minimized.

Copy link
@Calyhre

Calyhre Sep 10, 2019

If the format is known, why basing this on a mutation of the array and not directly destructure it to the variable you want? You would also avoid the not-very-clear content[0] later? Like this:

const [start, content] = args

Actually, why using an ...args at all here instead of listing the arguments (id, start, content)?

index = content.length;
}

const chars = this.note.split('');

This comment has been minimized.

Copy link
@Calyhre

Calyhre Sep 10, 2019

Wouldn't that be very costly in terms of memory and performance to create a potentially huge array of chars at each insert? Isn't there a way of avoiding this?


delete(id) {
console.log('DELETE', id);
store.set(id, undefined);

This comment has been minimized.

Copy link
@Calyhre

Calyhre Sep 10, 2019

You added to Storage a delete method that sets to null, wouldn't that be confusing and source of potential future issues to have duplicate yet different ways of deleting documents?

}

exists(id) {
return store.get(id) !== undefined;

This comment has been minimized.

Copy link
@Calyhre

Calyhre Sep 10, 2019

Depending on how you delete a document, via this controller or directly via Storage.delete, couldn't this also end up in issues later? One is setting the value to undefined and the later to null

}

delete(id) {
this.store[id] = null;

This comment has been minimized.

Copy link
@Calyhre

Calyhre Sep 10, 2019

This would still live in memory and take space wouldn't it?

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