feat: support CRUD on /schemas, /tables, /columns

[soedirgo]

Basic parameters for now, more specialized column properties to be added later.

[kiwicopple]

Looks good Bobbie. I think a couple of important notes on code structure:

There is a tendency to lump all your code into one big function. This actually detracts from the readability (and also testability). Compare one of your PATCH method to the PATCH method in schemas.js. All of the SQL queries in schemas.js are separated out, so it's clear when you skim down the function what is happening. Also it makes the whole thing more testable ( because they are pure functions).

Also readability can be gained by converting things like this:

assert.equal(
      tables.some((table) => `${table.schema}.${table.name}` === `public.test a`),
      true
    )

to this:

const testTableExists = tables.some((table) => `${table.schema}.${table.name}` === `public.test a`)
assert.equal(testTableExists, true)

This is aesthetically easier to read, and also the const naming gives new users a clear idea what your the function is doing. Even if the cognitive load of a .some() function is small, these things add up over time.

No need to change these now! But next update. (Make it work, Make it right, Make it fast )

[kiwicopple]

thing 😂

[soedirgo]

Oops, missed that one.

[kiwicopple]

can you help me to understand what this change does?

[kiwicopple]

If this is a breaking change, we shouldn't push it as the table sizes are used in Production already

[soedirgo]

This is part of the fix in f1de031 to quote identifiers with whitespace within the name. Creating a table with the name "foo bar", for example, will crash the server.

I figured I might as well quote everywhere I see identifiers are used since I find no reason not to.

[kiwicopple]

Nice one! very good catch

[soedirgo]

I agree that I should maintain code clarity by judiciously naming things. I remember something about the human brain only being able to juggle with 7? 9? different things at a time.

The assert.equal() example is spot on, I admit that I didn't pay much attention to the readability of the test cases (boo!). Point taken though, I'll be more careful from now on.

But I don't agree on the benefits of factoring out the helper functions as used in the PATCH methods.

• On readability

I think readability depends a lot on capturing the context of the original author's mind. While it seems easy to grasp schemas.js's PATCH through the names, I still need to inspect each function so I can be sure it's doing what I think it's doing. This means I have to go back and forth to the end of the file, instead of having everything contained within one page.

I'm also reluctant on factoring out things before seeing it occur 3 times. I view helper functions as mainly an abstraction device, and using it despite it only occuring in one place goes against that purpose. It also makes me conscious about other places the function is used, which adds to the mental burden.

• On testability

I agree on the benefits of pure functions and functional programming idioms, and I try minimize side effects where I can, but I'm not sure it applies here since it's pure function vs. no function. So I think factoring out the helpers is neutral at best wrt. testability.

I hope I don't come across as pretentious 😅 I care deeply about code quality, and it's something I can't get out of my head even when I'm just writing a prototype or a toy.

I don't have nearly as much experience as you, and the ability to write good code is one of the things I look forward to learn the most, so if you have a similar concern, please feel free to voice it out (and point me in the right direction). 😉

[kiwicopple]

sure thing, great that you're asking these questions. Note that all of these are one the "tacit" knowledge - I'm no great teacher, but I've spent plenty of time reading code.

Let's take your patch method:

router.patch('/:id', async (req, res) => {
  try {
    const [tableId, ordinalPos] = req.params.id.split('.')
    const getColumnQuery = SQL``
      .append(columns)
      .append(SQL` WHERE c.oid = ${tableId} AND ordinal_position = ${ordinalPos}`)
    const { schema, table, name: oldName } = (
      await RunQuery(req.headers.pg, getColumnQuery)
    ).data[0]

    const { name, type } = req.body as {
      name?: string
      type?: string
    }

    const query = `
BEGIN;
  ${
    type === undefined
      ? ''
      : `ALTER TABLE "${schema}"."${table}" ALTER COLUMN "${oldName}" SET DATA TYPE "${type}";`
  }
  ${
    name === undefined
      ? ''
      : `ALTER TABLE "${schema}"."${table}" RENAME COLUMN "${oldName}" TO "${name}";`
  }
COMMIT;`
    await RunQuery(req.headers.pg, query)

    const updated = (await RunQuery(req.headers.pg, getColumnQuery)).data[0]
    return res.status(200).json(updated)
  } catch (error) {
    console.log('throwing error', error)
    res.status(500).json({ error: 'Database error', status: 500 })
  }
})

Now imagine I update this method. After updating, it starts failing. OK, so where is the failure? I don't know, so I have to start up the app, run the code, probably scatter some console.log's. Or, perhaps I can refactor it a bit so I can isolate the failure? Perhaps the the failure is on getColumnQuery? So I pull this bit of code out into a function and then I write a little test on it:

const getColumnQuery = selectSingleColumnSql()

...

// Create a unit test just for this function
function selectSingleColumnSQL(tableId, ordinalPos) {
   return SQL``
      .append(columns) // Note that this is actually impure -> could be improved
      .append(SQL` WHERE c.oid = ${tableId} AND ordinal_position = ${ordinalPos}`)
}

So now a part of the code is isolated. I run the tests and I find that the code is broken inside this function. In the future if I break this code again, i know exactly where to go to fix it.

On readability: the name "selectSingleColumnSQL" above is much clearer to me than the content inside the function. Perhaps you disagree - my naming convention isn't great - but the concept is solid: Any sufficiently descriptive function name is going to be clearer than the content within that function.

Final point on readability: neatness matters. This is an odd one, but alignment and small, discrete units are always more digestible than big, unwieldy blocks.

Take this block of code, which is cognitively daunting:

const query = `
BEGIN;
  ${
    type === undefined
      ? ''
      : `ALTER TABLE "${schema}"."${table}" ALTER COLUMN "${oldName}" SET DATA TYPE "${type}";`
  }
  ${
    name === undefined
      ? ''
      : `ALTER TABLE "${schema}"."${table}" RENAME COLUMN "${oldName}" TO "${name}";`
  }
COMMIT;`

I can make this much neater:

const typeSql = !!type ? `ALTER TABLE "${schema}"."${table}" ALTER COLUMN "${oldName}" SET DATA TYPE "${type}";` : ''
const nameSql = !!name ? `ALTER TABLE "${schema}"."${table}" RENAME COLUMN "${oldName}" TO "${name}";` : ''
const query = `
BEGIN;
  ${typeSql}
  ${nameSql}
COMMIT;`

I've made it "neater" by lining everything up (this is why elastic tabstops looks so great).
I have only 2 levels of indentation rather than 4.
I can skim my eyes down the lefthand side of the page and get the key elements of the code, rather than darting my eyes everywhere.
Honestly, this is hard to explain why it's better, except to say that if you had structured it this way, I end up reading less code to understand it.

Once again, much of this is tacit. I hope it makes sense, but if it doesnt let me know and I'll try again

[kiwicopple]

I'm also reluctant on factoring out things before seeing it occur 3 times. I view helper functions as mainly an abstraction device, and using it despite it only occuring in one place goes against that purpose. It also makes me conscious about other places the function is used, which adds to the mental burden.

On this one - it's a good general rule. A poor abstraction is worse that a verbose function. But that is why I have abstracted into the same file, not into shared libs. The "removeSystemSchemas" are an example here. Almost every file has it, but they are each different. If we were using OOP, we might find that it's the perfect chance to implement an interface, and the abstracted function makes this simple to refactor.

[soedirgo]

Thanks for the explanations!

Take this block of code, which is cognitively daunting:

I agree. That piece of code was originally one line each. As it got longer, the formatter restructures it (I do format on save), and I didn't pay much attention to it afterwards.

I'm still a bit on the fence on the factoring-to-function part (I think giving it a well named variable is enough), but I think the fact that the functions are (more or less) pure functions avoids most of the drawbacks of doing this, and the default act should always be to follow the existing practices in a codebase 🙂

[kiwicopple]

factoring-to-function part

This actually started in the schemas.js because I used selectSingleSql() in multiple places. Then I had to change one to selectSingleName(). So it became a convention.

I see that in the /delete function you haven't made use of selectSingleSql(). Imagine we now had to change the way this operated (perhaps we remove 'sql-template-strings'), now we have the code broken in router.delete and selectSingleName()

Anyhow, as I say - this is a tacit thing. It seems important to me because I have a bad feeling about the 'sql-template-strings' library.

One more thing I thought about:

I still need to inspect each function so I can be sure it's doing what I think it's doing.

I actually don't do this - I don't read "all" the code. As the codebase grows this becomes impossible, so you just want an easy way to read the "flow" of the code.

[github-actions]

🎉 This PR is included in version 0.7.0 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀