Include the project role of a request token or user in project API #989

ikhoon · 2024-07-20T09:48:23Z

Motivation:

I think it is necessary to filter projects to which users belong or display their roles on the screen.

Currently, user information can't be obtained from /api/v1/projects API. So we need to call /api/v1/projects/${projectName} to get the metadata. That means 20 API calls should be made to list 20 projects in /app/projects. It will cause additional loads to servers and increase page loading time.

To resolve the N+1 query problem, I propose to add the role of a requested user to ProjectDto.

Modifications:

Add RepositoryListener abstraction that can be continuously updated when a new change is pushed to a Repository. Repository.watch() has a similar function but it can be updated only once.
Add ProjectMetadata to DefaultProject and use RepositoryListener to update ProjectMetadata in a project when new changes are made in metadata.json
Expose user roles on the Projects page of the new web application.
Refactor ControlPlanePlugin with RepositoryListener to simply the code.
Miscellaneous) Show a loading indicator when a page is loading.

Result:

The project role of a request token or user is now exposed in the project API.
Users can see their roles on the projects page.
Users can filter projects by their roles on the project page.

Motivation: I think it is necessary to filter projecs to which users belong or display their roles in the screen. Currently, a user information couldn't be obtained from `/api/v1/projects` API. So we need to call `/api/v1/projects/${projectName}` to get the metadata. That means 20 API calls should be made to list 20 projects in `/app/projects`. It will cause additional loads to servers and increase page loading time. To resolve the N+1 query problem, I propose to add a role of the requested user to `ProjectDto`. Modifications: - Add `RepositoryListener` abstraction that can be continuously updated when a new change is pushed to a `Repository`. `Repository.watch()` has a similar function but it can be updated only once. - Add `ProjectMetadata` to `DefaultProject` and use `RepositoryListener` to update `ProjectMetadata` in a project when new changes in `metadata.json` - Expose user roles in the Projects page of the new web application. - Refactor `ControlPlanePlugin` with `RepositoryListener` to simply the code Result: - The project role of a request token or user are now exposed in the project API. - Users can see their roles in the projects page. - Users can filter projects by their roles in the project page.

ikhoon · 2024-07-20T09:49:25Z

minwoox

Looks promising! 👍

common/src/main/java/com/linecorp/centraldogma/internal/api/v1/ProjectRoleDto.java

server/src/main/java/com/linecorp/centraldogma/server/internal/api/ProjectServiceV1.java

.../src/main/java/com/linecorp/centraldogma/server/internal/storage/project/DefaultProject.java

webapp/src/dogma/features/project/Projects.tsx

minwoox · 2024-07-24T13:03:51Z

webapp/src/dogma/features/project/Projects.tsx

-                    <MenuItemOption value="me">Created by me</MenuItemOption>
+                    <MenuItemOption value="ALL">All</MenuItemOption>
+                    <MenuItemOption value="MEMBER">I am a member</MenuItemOption>
+                    <MenuItemOption value="CREATOR">Created by me</MenuItemOption>


Do you have any easy way to test the UI in local?

I prefer the following steps:

Run ShiroCentralDogmaTestServer.main() using IntelliJ which is faster than ./gradlew runTestShiroServer.

If the server starts successfully, execute npm run develop and access http://127.0.0.1:3000.

If test is meant to run a unit test, we can run npm run test. I temporarily disable tests that fetch data from a mock server after upgrading dependencies. Because there were compatibility issues that I haven't solved yet.

Run ShiroCentralDogmaTestServer.main() using IntelliJ which is faster than ./gradlew runTestShiroServer.
If the server starts successfully, execute npm run develop and access http://127.0.0.1:3000.

That's what I was looking for. 😉

minwoox · 2024-07-24T13:05:59Z

xds/src/main/java/com/linecorp/centraldogma/xds/internal/ControlPlanePlugin.java

-                           watchDogmaRepository(repositoryManager, entry.revision());
-                           return null;
-                       }, CONTROL_PLANE_EXECUTOR);
+        dogmaRepository.addListener(RepositoryListener.of(MetadataService.METADATA_JSON, entries -> {


xds/src/main/java/com/linecorp/centraldogma/xds/internal/ControlPlanePlugin.java

jrhee17

Left some minor comments but looks good to me 👍 👍 👍

common/src/main/java/com/linecorp/centraldogma/common/ProjectRole.java

jrhee17 · 2024-07-26T04:00:52Z

...er/src/main/java/com/linecorp/centraldogma/server/storage/repository/RepositoryListener.java

+import com.linecorp.centraldogma.common.Entry;
+import com.linecorp.centraldogma.common.Query;
+
+import io.netty.util.internal.shaded.org.jctools.queues.MessagePassingQueue.Consumer;


I guess you meant to use jdk's consumer

jrhee17 · 2024-07-26T06:08:03Z

common/src/main/java/com/linecorp/centraldogma/internal/api/v1/ProjectDto.java

+                      @JsonProperty("creator") @Nullable Author creator,
+                      @JsonProperty("url") @Nullable String url,
+                      @JsonProperty("userRole") @Nullable ProjectRole userRole,
+                      @JsonProperty("createdAt") @Nullable String createdAt) {


Question) Even in tests, is there a need for the nullable contract since server-side always seems to fill in these fields?

This constructor was added for deserializing the API responses in tests.

centraldogma/server/src/test/java/com/linecorp/centraldogma/server/internal/api/ProjectServiceV1ListProjectTest.java

Lines 339 to 340 in 11e03be

.asJson(new TypeReference<List<ProjectDto>>() {})

.execute();

It wasn't used for ?status=removed responses but made them nullable for future usage.

This constructor was added for deserializing the API responses in tests.

I understand that. I meant even in the test you linked aren't the fields always being filled in (non-null) anyways?

If it is indeed nullable, I guess it's fine to leave as-is

you linked aren't the fields always being filled in (non-null) anyways?

Right. They are non-null for normal projects. The values are nullable for removed projects.

centraldogma/server/src/main/java/com/linecorp/centraldogma/server/internal/api/ProjectServiceV1.java

Lines 84 to 86 in c0673c1

return CompletableFuture.supplyAsync(() -> projectApiManager.listRemovedProjects().keySet()

.stream()

.map(ProjectDto::new)

jrhee17 · 2024-07-26T06:08:37Z

common/src/main/java/com/linecorp/centraldogma/internal/api/v1/ProjectDto.java

@@ -74,9 +96,9 @@ public String url() {
    }

    @Nullable
-    @JsonProperty("reposUrl")


note: checked that this field doesn't seem to be used in the previous UI also, which guarantees backwards compatibility

jrhee17 · 2024-07-26T06:09:48Z

server/src/main/java/com/linecorp/centraldogma/server/internal/api/TokenService.java

@@ -103,7 +104,7 @@ public CompletableFuture<Collection<Token>> listTokens(User loginUser) {
    @StatusCode(201)
    @ResponseConverter(CreateApiResponseConverter.class)
    public CompletableFuture<HttpResult<Token>> createToken(@Param String appId,
-                                                            @Param boolean isAdmin,
+                                                            @Param @Default("false") boolean isAdmin,


Question) Is this related to changes in this PR? Or is this a separate improvement?

It is not related to this PR.
I've fixed it in this PR together since it looked like a trivial change.

.../src/main/java/com/linecorp/centraldogma/server/internal/storage/project/DefaultProject.java

jrhee17 · 2024-07-26T06:13:17Z

server/src/main/java/com/linecorp/centraldogma/server/internal/api/ProjectServiceV1.java

+            return ProjectRole.OWNER;
+        }
+
+        final ProjectMetadata metadata = project.metadata();


Question) I believe Project#metadata is updated asynchronously. This means if a user changes his/her metadata and directly navigates to /projects, I think it is possible that the change may not be reflected depending on whether the find in the listener has been invoked or not.

I think it should be fine since listing "my projects" can be considered non-critical, but just wanted to make sure if this is an intentional design choice as opposed to just using a write-through cache. (I haven't checked if there are any UI paths which update metadata and navigate to /projects immediately)

Good point. As you know, the consistent model of Central Dogma is eventual consistency and does not provide a transaction mechanism.

Even before this change, users may see old content right after a commit is pushed successfully. ReplicationLagTolerantCentralDogma is a workaround to make up for the downside of eventual consistency.

I see, I understood that even if a user creates a project and directly navigates to the main page he/she may not see her project highlighted.

When I tested, I was able to see newly created projects in most cases. But it may happen if there is a significant delay. Let's leave it a known issue for now.

minwoox

Thanks! 👍 👍 👍

minwoox · 2024-08-06T08:16:46Z

common/src/main/java/com/linecorp/centraldogma/internal/api/v1/ProjectDto.java

        this.name = requireNonNull(name, "name");
        this.creator = requireNonNull(creator, "creator");
        createdAt = ISO_INSTANT.format(Instant.ofEpochMilli(creationTimeMillis));
        url = PROJECTS_PREFIX + '/' + name;
+        this.userRole = userRole;


minwoox · 2024-08-06T08:22:51Z

.../src/main/java/com/linecorp/centraldogma/server/internal/storage/project/DefaultProject.java

+                                     Change.ofJsonUpsert(METADATA_JSON, Jackson.valueToTree(metadata)))
+                             .join();
+            lastMetadataRevision = result.revision();
+            this.projectMetadata = metadata;


Suggested change

this.projectMetadata = metadata;

projectMetadata = metadata;

minwoox · 2024-08-06T08:24:53Z

.../src/main/java/com/linecorp/centraldogma/server/internal/storage/project/DefaultProject.java

            } else {
                creationTimeMillis = repos.get(REPO_DOGMA).creationTimeMillis();
                author = repos.get(REPO_DOGMA).author();
            }
            success = true;
+        } catch (Exception e) {
+            throw new CentralDogmaException("failed to initialize internal repositories", e);


nit:

Suggested change

throw new CentralDogmaException("failed to initialize internal repositories", e);

throw new CentralDogmaException("failed to initialize internal repositories of " + name, e);

Please also add the project name to the exception message below.

minwoox · 2024-08-06T08:29:01Z

.../src/main/java/com/linecorp/centraldogma/server/internal/storage/project/DefaultProject.java

+            }
+
+            final Revision lastRevision = entry.revision();
+            if (lastRevision.compareTo(lastMetadataRevision) <= 0) {


nit: assert lastMetadataRevision != null;
We might want to assign it to a local variable not to access the field twice.

ikhoon added improvement new feature labels Jul 20, 2024

ikhoon added this to the 0.67.1 milestone Jul 20, 2024

ikhoon requested review from jrhee17, minwoox and trustin as code owners July 20, 2024 09:48

ikhoon and others added 3 commits July 21, 2024 22:11

clean up

133c86a

Make admin as owner

ba7ea06

Merge branch 'main' into project-user-role

3044ba1

minwoox modified the milestones: 0.67.1, 0.67.2 Jul 23, 2024

ikhoon modified the milestones: 0.67.2, 0.68.0 Jul 23, 2024

minwoox modified the milestones: 0.68.0, 0.69.0 Jul 23, 2024

minwoox reviewed Jul 24, 2024

View reviewed changes

move ProjectRole to common

ac9bc2b

jrhee17 approved these changes Jul 26, 2024

View reviewed changes

ikhoon added the breaking change label Jul 27, 2024

ikhoon added 5 commits July 27, 2024 11:27

Merge branch 'main' into project-user-role

d13238f

Merge branch 'main' into project-user-role

454e692

address comments

11e03be

lint

c0673c1

Merge branch 'main' into project-user-role

c63128b

minwoox approved these changes Aug 6, 2024

View reviewed changes

Address comments by @minwoox

ac3a1a9

minwoox merged commit c615ba5 into line:main Aug 8, 2024
10 checks passed

ikhoon deleted the project-user-role branch August 30, 2024 09:10

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Include the project role of a request token or user in project API #989

Include the project role of a request token or user in project API #989

ikhoon commented Jul 20, 2024 •

edited

Loading

ikhoon commented Jul 20, 2024

minwoox left a comment

minwoox Jul 24, 2024

ikhoon Jul 24, 2024

minwoox Jul 25, 2024

minwoox Jul 24, 2024

jrhee17 left a comment

jrhee17 Jul 26, 2024

ikhoon Jul 27, 2024

jrhee17 Jul 26, 2024

ikhoon Jul 30, 2024

jrhee17 Jul 30, 2024 •

edited

Loading

ikhoon Jul 31, 2024

jrhee17 Jul 26, 2024

jrhee17 Jul 26, 2024

ikhoon Jul 27, 2024

jrhee17 Jul 26, 2024

ikhoon Jul 27, 2024 •

edited

Loading

jrhee17 Aug 8, 2024

ikhoon Aug 8, 2024

minwoox left a comment

minwoox Aug 6, 2024

minwoox Aug 6, 2024

minwoox Aug 6, 2024

minwoox Aug 6, 2024

	.asJson(new TypeReference<List<ProjectDto>>() {})
	.execute();

	return CompletableFuture.supplyAsync(() -> projectApiManager.listRemovedProjects().keySet()
	.stream()
	.map(ProjectDto::new)

	throw new CentralDogmaException("failed to initialize internal repositories", e);
	throw new CentralDogmaException("failed to initialize internal repositories of " + name, e);

Include the project role of a request token or user in project API #989

Include the project role of a request token or user in project API #989

Conversation

ikhoon commented Jul 20, 2024 • edited Loading

ikhoon commented Jul 20, 2024

minwoox left a 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

jrhee17 left a 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

jrhee17 Jul 30, 2024 • edited Loading

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

ikhoon Jul 27, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

minwoox left a 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

ikhoon commented Jul 20, 2024 •

edited

Loading

jrhee17 Jul 30, 2024 •

edited

Loading

ikhoon Jul 27, 2024 •

edited

Loading