tweak(adminPanel): RN-1273: Map overlay relations by code #5646

jaskfla · 2024-05-17T01:50:04Z

Issue RN-1273: Map overlay group relation id to code

Changes

Adds child_code to MapOverlayGroupRelation at the model level
Adds Child code column to Map overlay group relations page in Admin Panel

~~Model changes based largely off #4270, which was closed without merging~~ Not anymore

packages/database/src/modelClasses/MapOverlayGroupRelation.js

packages/central-server/src/apiV2/GETHandler/GETHandler.js

packages/database/src/DatabaseRecord.js

jaskfla · 2024-05-30T01:41:42Z

packages/database/src/TupaiaDatabase.js

 function getColSelector(connection, inputColStr) {
  const jsonOperatorPattern = /->>?/g;
-  if (!jsonOperatorPattern.test(inputColStr)) return inputColStr;
+  if (jsonOperatorPattern.test(inputColStr)) {
+    const params = inputColStr.split(jsonOperatorPattern);
+    const allButFirst = params.slice(1);
+    const lastIndexOfLookupAsText = inputColStr.lastIndexOf('->>');
+    const lastIndexOfLookupAsJson = inputColStr.lastIndexOf('->');
+    const selector = lastIndexOfLookupAsText >= lastIndexOfLookupAsJson ? '#>>' : '#>';
+
+    // Turn `config->item->>colour` into `config #>> '{item,colour}'`
+    // For some reason, Knex fails when we try to convert it to `config->'item'->>'colour'`
+    return connection.raw(`?? ${selector} '{${allButFirst.map(() => '??').join(',')}}'`, params);
+  }

-  const params = inputColStr.split(jsonOperatorPattern);
-  const allButFirst = params.slice(1);
-  const lastIndexOfLookupAsText = inputColStr.lastIndexOf('->>');
-  const lastIndexOfLookupAsJson = inputColStr.lastIndexOf('->');
-  const selector = lastIndexOfLookupAsText >= lastIndexOfLookupAsJson ? '#>>' : '#>';
+  /**
+   * Special handling of COALESCE() - one of the {@link supportedFunctions} - to treat its arguments
+   * as identifiers individually rather than trying to treat ‘COALESCE(foo, bar)’ as a single
+   * identifier.
+   */
+  const coalescePattern = /^COALESCE\(.+\)$/;
+  if (coalescePattern.test(inputColStr)) {
+    const [, argsString] = inputColStr.match(/^COALESCE\((.+)\)$/);
+    const bindings = argsString.split(',').map(arg => arg.trim());
+    const identifiers = bindings.map(() => '??');
+
+    return connection.raw(`COALESCE(${identifiers})`, bindings);
+  }

-  // Turn `config->item->>colour` into `config #>> '{item,colour}'`
-  // For some reason, Knex fails when we try to convert it to `config->'item'->>'colour'`
-  return connection.raw(`?? ${selector} '{${allButFirst.map(() => '??').join(',')}}'`, params);
+  return inputColStr;
 }


Two notes:

This implementation assumes we never have a column selector which both calls COALESCE() and uses JSON selectors. If that happens, this algorithm fails. Is this a concern?

Should I make it iterate through supportedFunctions to handle any function in that array?

Ah that's fine I think

Probably worth it I think! Would make this functionality a lot more future proof if we want to make more use of db functions. If you're happy to 🙏

Hm, it occurs to me that, given some supportedFunction, it would be difficult to know whether to treat its arguments as ? values or ?? identifiers

I could just build in an assumption and always use ??, but this would silently break any attempt to use a function that takes, say, a number. For example, if we have ROUND(foo, 2) Knex would complain that there’s no column called "2". As far as I can tell, we would need to hard-code it to do ROUND(??, ?)

I can’t think of a clean way about this, so have left some comments about why COALESCE gets special handling, and that this might extend to future supported functions too. Would be thrilled if you can think of something obvious I’ve missed, but going to go ahead as-is for now

packages/admin-panel/src/routes/visualisations/mapOverlayGroupRelations.js

packages/database/src/DatabaseModel.js

packages/database/src/modelClasses/MapOverlayGroupRelation.js

…names

rohan-bes

Awesome stuff @jaskfla 🎉

This issue is definitely one of those things that looks super easy on the surface, but is deceptively tricky. Thanks for pushing through with it and really happy with the end result (I feel like it tidies up and makes a few things more consistent with the CRUDHandler
and DatabaseModel behaviour)

Just a comment to ask if we can loop over the supported functions when sanitizing the WHERE clause identifier, but pre-approving

rohan-bes · 2024-06-14T05:38:34Z

packages/database/src/TupaiaDatabase.js

 function getColSelector(connection, inputColStr) {
  const jsonOperatorPattern = /->>?/g;
-  if (!jsonOperatorPattern.test(inputColStr)) return inputColStr;
+  if (jsonOperatorPattern.test(inputColStr)) {
+    const params = inputColStr.split(jsonOperatorPattern);
+    const allButFirst = params.slice(1);
+    const lastIndexOfLookupAsText = inputColStr.lastIndexOf('->>');
+    const lastIndexOfLookupAsJson = inputColStr.lastIndexOf('->');
+    const selector = lastIndexOfLookupAsText >= lastIndexOfLookupAsJson ? '#>>' : '#>';
+
+    // Turn `config->item->>colour` into `config #>> '{item,colour}'`
+    // For some reason, Knex fails when we try to convert it to `config->'item'->>'colour'`
+    return connection.raw(`?? ${selector} '{${allButFirst.map(() => '??').join(',')}}'`, params);
+  }

-  const params = inputColStr.split(jsonOperatorPattern);
-  const allButFirst = params.slice(1);
-  const lastIndexOfLookupAsText = inputColStr.lastIndexOf('->>');
-  const lastIndexOfLookupAsJson = inputColStr.lastIndexOf('->');
-  const selector = lastIndexOfLookupAsText >= lastIndexOfLookupAsJson ? '#>>' : '#>';
+  /**
+   * Special handling of COALESCE() - one of the {@link supportedFunctions} - to treat its arguments
+   * as identifiers individually rather than trying to treat ‘COALESCE(foo, bar)’ as a single
+   * identifier.
+   */
+  const coalescePattern = /^COALESCE\(.+\)$/;
+  if (coalescePattern.test(inputColStr)) {
+    const [, argsString] = inputColStr.match(/^COALESCE\((.+)\)$/);
+    const bindings = argsString.split(',').map(arg => arg.trim());
+    const identifiers = bindings.map(() => '??');
+
+    return connection.raw(`COALESCE(${identifiers})`, bindings);
+  }

-  // Turn `config->item->>colour` into `config #>> '{item,colour}'`
-  // For some reason, Knex fails when we try to convert it to `config->'item'->>'colour'`
-  return connection.raw(`?? ${selector} '{${allButFirst.map(() => '??').join(',')}}'`, params);
+  return inputColStr;
 }


Ah that's fine I think

Probably worth it I think! Would make this functionality a lot more future proof if we want to make more use of db functions. If you're happy to 🙏

jaskfla added 3 commits May 16, 2024 15:55

child_code in MapOverlayGroupRelations.find

6e41add

use generateInstance

7dcb145

Merge branch 'dev' into rn-1273-overlay-relations-by-code

bebda01

jaskfla changed the title ~~RN-1273: Map overlay relations by code~~ tweak(adminPanel): RN-1273: Map overlay relations by code May 17, 2024

comment new methods

e544e17

jaskfla commented May 17, 2024

View reviewed changes

packages/database/src/modelClasses/MapOverlayGroupRelation.js Outdated Show resolved Hide resolved

jaskfla and others added 24 commits May 17, 2024 15:38

reorder a few symbols

e83030b

temp: clearer console log

aebf87e

Merge branch 'dev' into rn-1273-overlay-relations-by-code

a3e699f

use TSDoc

bbc137b

avoid arrow notation

b9af765

consistent param names

641cfa0

remove console logs

2f9f176

private remark comment

ccb7850

clearer comment

395eed3

Merge branch 'dev' into rn-1273-overlay-relations-by-code

6074dad

format JSDoc

077929f

update error log

b65b533

document mapOverlayGroupRelations endpoints

b895ce2

use static joins

3454288

fix invalid criteria

7f8a589

restore criteria, fix invalid options?

506ca1a

Switch to using 'customColumnSelector' for child_code field

b7c1261

remove overriding find and generateInstance

a77f4ee

Merge branch 'dev' into rn-1273-overlay-relations-by-code

fdcd731

Merge branch 'dev' into rn-1273-overlay-relations-by-code

02b448a

Merge branch 'dev' into rn-1273-overlay-relations-by-code

5641660

use processColumnSelector

0afce39

Merge branch 'dev' into rn-1273-overlay-relations-by-code

022dd41

adjust inline comment

bd42c93

jaskfla added 7 commits May 27, 2024 12:58

clarifying JSDoc

8f4fc41

update comment, use for…of

de9be76

Merge branch 'dev' into rn-1273-overlay-relations-by-code

9d42431

handling for COALESCE

a7fd2cb

Merge branch 'dev' into rn-1273-overlay-relations-by-code

a4ac96e

fix import

efc435f

update Admin Panel columns, edit/create configs

1ee9789

jaskfla commented May 30, 2024

View reviewed changes

packages/central-server/src/apiV2/GETHandler/GETHandler.js Show resolved Hide resolved

remove console log

8ee3ac6

jaskfla commented May 30, 2024

View reviewed changes

packages/database/src/DatabaseRecord.js Outdated Show resolved Hide resolved

jaskfla marked this pull request as ready for review May 30, 2024 01:34

jaskfla commented May 30, 2024

View reviewed changes

packages/admin-panel/src/routes/visualisations/mapOverlayGroupRelations.js Show resolved Hide resolved

Merge branch 'dev' into rn-1273-overlay-relations-by-code

7f4f4e0

jaskfla commented May 31, 2024

View reviewed changes

packages/database/src/DatabaseModel.js Outdated Show resolved Hide resolved

jaskfla added 2 commits May 31, 2024 15:04

fix cascading test failures

b1999f2

Merge branch 'dev' into rn-1273-overlay-relations-by-code

2e84b4c

jaskfla commented Jun 6, 2024

View reviewed changes

packages/database/src/modelClasses/MapOverlayGroupRelation.js Show resolved Hide resolved

Merge branch 'dev' into rn-1273-overlay-relations-by-code

7256ef1

rohan-bes force-pushed the rn-1273-overlay-relations-by-code branch 2 times, most recently from 139b415 to 2809015 Compare June 14, 2024 05:13

RN-1273: Ensure DatabaseModel find methods use fully qualified field …

8920597

…names

rohan-bes force-pushed the rn-1273-overlay-relations-by-code branch from 2809015 to 8920597 Compare June 14, 2024 05:22

rohan-bes approved these changes Jun 14, 2024

View reviewed changes

jaskfla added 3 commits June 17, 2024 12:25

comment unhandled edge case

1fb45fe

Merge branch 'dev' into rn-1273-overlay-relations-by-code

74d75dd

beefier TSDoc

63ae3a3

jaskfla merged commit 18b4128 into dev Jun 20, 2024
44 checks passed

jaskfla deleted the rn-1273-overlay-relations-by-code branch June 20, 2024 02:12

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

tweak(adminPanel): RN-1273: Map overlay relations by code #5646

tweak(adminPanel): RN-1273: Map overlay relations by code #5646

jaskfla commented May 17, 2024 •

edited

Loading

jaskfla May 30, 2024 •

edited

Loading

rohan-bes Jun 14, 2024

jaskfla Jun 17, 2024 •

edited

Loading

rohan-bes left a comment

rohan-bes Jun 14, 2024

tweak(adminPanel): RN-1273: Map overlay relations by code #5646

tweak(adminPanel): RN-1273: Map overlay relations by code #5646

Conversation

jaskfla commented May 17, 2024 • edited Loading

Issue RN-1273: Map overlay group relation id to code

Changes

jaskfla May 30, 2024 • edited Loading

Choose a reason for hiding this comment

rohan-bes Jun 14, 2024

Choose a reason for hiding this comment

jaskfla Jun 17, 2024 • edited Loading

Choose a reason for hiding this comment

rohan-bes left a comment

Choose a reason for hiding this comment

rohan-bes Jun 14, 2024

Choose a reason for hiding this comment

jaskfla commented May 17, 2024 •

edited

Loading

jaskfla May 30, 2024 •

edited

Loading

jaskfla Jun 17, 2024 •

edited

Loading