fix: group cleanup #4334

kwasniew · 2023-07-24T08:26:23Z

About the changes

Group API cleanup triggered by the OpenAPI improvements
Replacing 500 errors with 400 errors when foreign key violations happen (no role, no user)
Separating write/create model and read model for groups
Removing transaction boundaries from the store to the controller in group-controller to have the same pattern across the entire codebase and possibly span multiple stores

Important files

Discussion points

sonatype-lift · 2023-07-24T08:26:26Z

Sonatype Lift is retiring

Sonatype Lift will be retiring on Sep 12, 2023, with its analysis stopping on Aug 12, 2023. We understand that this news may come as a disappointment, and Sonatype is committed to helping you transition off it seamlessly. If you’d like to retain your data, please export your issues from the web console.
We are extremely grateful and thank you for your support over the years.

📖 Read about the impacts and timeline

vercel · 2023-07-24T08:26:29Z

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name	Status	Preview	Comments	Updated (UTC)
unleash-docs	✅ Ready (Inspect)	Visit Preview	💬 Add feedback	Jul 24, 2023 8:33am
unleash-monorepo-frontend	✅ Ready (Inspect)	Visit Preview	💬 Add feedback	Jul 24, 2023 8:33am

kwasniew · 2023-07-24T08:27:29Z

src/lib/db/group-store.ts

+                .returning(GROUP_COLUMNS);
+
+            return rowToGroup(rows[0]);
+        } catch (error) {


What if the role id does not exist? Currently we don't check if the role is a root role, but we don't have this check before either.

If the role doesn't exist at all, it should fail, which I think this now does, thanks to this PR. If the role is not a root role then this will succeed but effectively be a no-op. Probably needs some love but I think this PR takes us forward

Exactly, this PR prevent doesn't allow to pass nonexistent role ids and fails with 400 on those

kwasniew · 2023-07-24T08:27:44Z

src/lib/db/group-store.ts

+                .returning('*');
+            return rowToGroup(row[0]);
+        } catch (error) {
+            if (


same as update

kwasniew · 2023-07-24T08:29:35Z

src/lib/db/group-store.ts

+            });
+            return await this.db.batchInsert(T.GROUP_USER, rows);
+        } catch (error) {
+            if (


If user list has invalid user id. We could check this upfront when we validate group in the service but it was more invasive since we'd need to inject read models from the user subdomain. I don't mind having this code here since it's DB related code wrapped in a DB specific store so the details are not leaking out.

Yeah agreed, I think it's fine living in this logical layer

kwasniew · 2023-07-24T08:29:59Z

src/lib/features/group/createGroupService.ts

+import { AccountStore } from '../../db/account-store';
+import EventStore from '../../db/event-store';
+
+export const createGroupService = (


needed for transaction support so that we new up a service per DB transaction

kwasniew · 2023-07-24T08:30:14Z

src/lib/openapi/spec/create-group-schema.ts

+import { FromSchema } from 'json-schema-to-ts';
+import { groupSchema } from './group-schema';
+
+export const createGroupSchema = {


separate schema from the read schema we were sharing before

kwasniew · 2023-07-24T08:31:00Z

src/lib/openapi/spec/create-group-schema.ts

+                    user: {
+                        type: 'object',
+                        description: 'A minimal user object',
+                        required: ['id'],


I was checking the UI and we don't send full users as the previous schema indicated but only the ids.

kwasniew · 2023-07-24T08:31:37Z

src/lib/types/group.ts

@@ -33,6 +33,11 @@ export interface IGroupModel extends IGroup {
    projects?: string[];
 }

+export interface ICreateGroupModel extends IGroup {


new TS model reflecting openapi schema change. Users are optional and each user is represented by id only

kwasniew · 2023-07-24T08:31:53Z

src/lib/types/services.ts

@@ -96,4 +96,5 @@ export interface IUnleashServices {
    transactionalFeatureToggleService: (
        db: Knex.Transaction,
    ) => FeatureToggleService;
+    transactionalGroupService: (db: Knex.Transaction) => GroupService;


we'll use it in the enterprise controller

kwasniew · 2023-07-24T08:32:10Z

src/lib/types/stores/group-store.ts

@@ -38,20 +38,20 @@ export interface IGroupStore extends Store<IGroup, number> {

    updateGroupUsers(
        groupId: number,
-        newUsers: IGroupUserModel[],
+        newUsers: ICreateGroupUserModel[],


write/create model decoupled from the read model

kwasniew · 2023-07-24T08:32:28Z

src/test/e2e/services/group-service.e2e.test.ts

@@ -124,3 +124,26 @@ test('adding a root role to a group with a project role should fail', async () =

    expect.assertions(1);
 });
+
+test('adding a nonexistent role to a group should fail', async () => {


confirming the try/catch I added in the store works

kwasniew · 2023-07-24T08:36:11Z

src/lib/db/group-store.ts

-                created_by: userName,
-            };
-        });
-        return (transaction || this.db).batchInsert(T.GROUP_USER, rows);


removing transactions from the store and moving them to the controller/around the group service in the enterprise repo

sighphyre

LGTM

fix: group cleanup

a180c3c

github-actions bot assigned kwasniew Jul 24, 2023

kwasniew commented Jul 24, 2023

View reviewed changes

src/lib/db/group-store.ts

.returning('*');

return rowToGroup(row[0]);

} catch (error) {

if (

Copy link

Contributor Author

kwasniew Jul 24, 2023

Choose a reason for hiding this comment

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

same as update

kwasniew commented Jul 24, 2023

View reviewed changes

vercel bot deployed to Preview – unleash-docs July 24, 2023 08:31 View deployment

kwasniew commented Jul 24, 2023

View reviewed changes

vercel bot deployed to Preview – unleash-monorepo-frontend July 24, 2023 08:33 View deployment

kwasniew commented Jul 24, 2023

View reviewed changes

sighphyre approved these changes Jul 24, 2023

View reviewed changes

kwasniew merged commit 5de4958 into main Jul 24, 2023
19 checks passed

kwasniew deleted the group-openapi-cleanup branch July 24, 2023 09:05

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: group cleanup #4334

fix: group cleanup #4334

kwasniew commented Jul 24, 2023 •

edited

sonatype-lift bot commented Jul 24, 2023

vercel bot commented Jul 24, 2023 •

edited

kwasniew Jul 24, 2023 •

edited

sighphyre Jul 24, 2023

kwasniew Jul 24, 2023

kwasniew Jul 24, 2023

kwasniew Jul 24, 2023

sighphyre Jul 24, 2023

kwasniew Jul 24, 2023

kwasniew Jul 24, 2023

kwasniew Jul 24, 2023

kwasniew Jul 24, 2023

kwasniew Jul 24, 2023

kwasniew Jul 24, 2023

kwasniew Jul 24, 2023

kwasniew Jul 24, 2023

sighphyre left a comment

fix: group cleanup #4334

fix: group cleanup #4334

Conversation

kwasniew commented Jul 24, 2023 • edited

About the changes

Important files

Discussion points

sonatype-lift bot commented Jul 24, 2023

Sonatype Lift is retiring

vercel bot commented Jul 24, 2023 • edited

kwasniew Jul 24, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sighphyre left a comment

Choose a reason for hiding this comment

kwasniew commented Jul 24, 2023 •

edited

vercel bot commented Jul 24, 2023 •

edited

kwasniew Jul 24, 2023 •

edited