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

refactor: contact list endpoint #488

Draft
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

robert-bo-davis
Copy link
Collaborator

@robert-bo-davis robert-bo-davis commented Oct 27, 2023

Description

This is a rough POC of the start of using a minimal list of contact properties to load the profile relationships. It abstracts away the "List properties" of contacts so they can be used used to generate list SQL.

It is a very early POC, but I wanted to get input on the pattern before I move much further.

Relationships are one of the places where the "domain" in the sql layer starts to get a little fuzzy.

Are there other similar patterns in code that I've missed that I should be emulating?

Checklist

  • Corresponding issue has been opened
  • New tests added

Related Issues

Fixes #....

Verification steps

Copy link
Collaborator

@stephenhand stephenhand left a comment

Choose a reason for hiding this comment

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

Couple of comments, but overall the approach is fine by me, similar to some of the sql building we are already doing

* @returns {string} - SQL snippet
*/
export const fieldListToSql = (table: string, fields: readonly string[]): string =>
fields.map(prop => `"${table}"."${prop}"`).join(', ');
Copy link
Collaborator

Choose a reason for hiding this comment

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

https://github.com/vitaly-t/pg-promise#sql-names - pg-promise supports passing SQL names as arrays into its substitution syntax, so hand rolling this might be unnecessary

* @param {string[]} properties - array of property names in dot notation
* @returns {string} - SQL snippet
*/
export const objectNotationToBuildObjectSql = (
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do we need to be so parsimonious with the JSON we read at the SQL level? We will be reading the whole object off disk anyway, so I assume this is to save on the amount of data we sends back to the client?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

In my mind, this pattern is a tiny initial step towards the rough permission division of list, view, edit. In the simple version of this pattern there are minimal "per field" permissions that get VERY complicated VERY quickly. The simplest form is just to leave out the data someone doesn't have access to from the list permission and reject their request for the view endpoint that would load data they don't have access to. I think other terminology is being used on the team, like "limited view" vs "full view", but the concept, in my mind, is similar.

This is the very tip of the iceberg related to a way of enforcing those permissions at the SQL level via abstraction and configuration in code.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Whilst definitely think this approach is the way to go on the table field level, I just wonder whether the added complexity of generating Postgres JSON SQL queries vs doing client side transformations is worth the benefit. If we did those client transforms at the lowest layer it would still be abstracted from the perspective of the rest of the system I think?

I'm ok with either approach, I think both would work, just wanted to see why you thought filtering the JSON blobs in SQL was the better approach in this case

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants