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

Testing BA-MBSE Persistence Refactor Merge #259

Merged
merged 66 commits into from
Apr 9, 2024

Conversation

HuiJun
Copy link
Collaborator

@HuiJun HuiJun commented Feb 7, 2024

deconflicted branch of #235

}
statement.executeUpdate(connection.nativeSQL("DROP DATABASE " + databaseProjectString(project)));
statement.executeUpdate(connection.nativeSQL("DROP DATABASE " + databaseProjectString(projectId)));

Check failure

Code scanning / CodeQL

Query built from user-controlled sources

This query depends on a [user-provided value](1). This query depends on a [user-provided value](2).
Copy link
Collaborator

@dlamoris dlamoris left a comment

Choose a reason for hiding this comment

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

have not went over permissions related changes yet
things to check:

  • some performance optimizations in federated classes where it's getting entire commit json instead of just commit info from db
  • updating deleted elements, deleted key in json
  • series of inits across sub/superclasses during element get/post, check logic if element post has deletes as well as updates


NodeChangeInfo commitChanges(NodeChangeInfo nodeChangeInfo);

NodeGetInfo findById(String elementId, String refId, String commitId, String id);
Copy link
Collaborator

Choose a reason for hiding this comment

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

first argument is projectId, last argument is elementId

Copy link
Collaborator

Choose a reason for hiding this comment

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

fixed in #260

@@ -80,38 +82,37 @@ public ElementsResponse read(String projectId, String refId, ElementsRequest req
}

@Override
public void extraProcessPostedElement(ElementJson element, Node node, NodeChangeInfo info) {
node.setNodeType(cameoHelper.getNodeType(element).getValue());
Copy link
Collaborator

Choose a reason for hiding this comment

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

check

Copy link
Collaborator

Choose a reason for hiding this comment

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

String commitId = params.getOrDefault("commitId", null);
ContextHolder.setContext(projectId, refId);
NodeGetInfo info = nodeGetHelper.processGetJson(req.getElements(), commitId, this);
String commitId = params.getOrDefault(CameoConstants.COMMITID, null);
Copy link
Collaborator

Choose a reason for hiding this comment

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

commitId is a generic request param, not cameo specific one

Copy link
Collaborator

Choose a reason for hiding this comment

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

fixed in #260

ViewService viewService = genericServiceFactory.getServiceForSchema( ViewService.class , getProjectType(projectId));
ElementsResponse res = viewService.createOrUpdate(projectId, refId, req, params, auth.getName());
// ToDo : the following is just a workaround till the indexing part is synchronized W
try {
Copy link
Collaborator

Choose a reason for hiding this comment

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

check

Copy link
Collaborator

Choose a reason for hiding this comment

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

fixed in #260 think this was because the added commit check done during read

if (keyHolder.getKey() != null) {
node.setId(keyHolder.getKey().longValue());
} else {
throw new NotFoundException("Key value was null");
Copy link
Collaborator

Choose a reason for hiding this comment

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

transaction?

throw new InternalErrorException("Unexpected NodeChangeInfo type in FederatedNodeChangeDomain");
}
Node n = ((FederatedNodeChangeInfo) info).getExistingNodeMap().get(element.getId());
if(n != null) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

remove

Copy link
Collaborator

Choose a reason for hiding this comment

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

fixed in #260

}
return res;
}

@Override
public void extraProcessPostedElement(ElementJson element, Node node, NodeChangeInfo info) {
public void extraProcessPostedElement(NodeChangeInfo info, ElementJson element) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

there shouldn't be any _childViews posted starting with ve 5, can indicate incompatibility with ve < 5 going forward

userPersistence.save(user);
//Add groups to user

Set<GroupJson> addGroups = new HashSet<>();
Copy link
Collaborator

Choose a reason for hiding this comment

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

actually don't need to be syncing local db with ldap groups, this can be taken out , just the granted authority (and token) need to have the groups

Copy link
Collaborator

Choose a reason for hiding this comment

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

will fix in a separate issue/pr

List<ElementJson> groups = getNodePersistence().findAllByNodeType(projectId, refId, commitId,
CameoNodeType.GROUP.getValue());

ElementsResponse res = this.read(projectId, refId, buildRequestFromJsons(groups), params);
Copy link
Collaborator

Choose a reason for hiding this comment

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

groups already is the right list of json

Copy link
Collaborator

Choose a reason for hiding this comment

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

fixed in #260

@@ -75,42 +82,47 @@ public void addChildViews(ElementsResponse res, Map<String, String> params) {
}

public ElementsResponse getGroups(String projectId, String refId, Map<String, String> params) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

add override

}
res.getCommits().addAll(commitIndex.elementHistory(elementId, commitIds));
res.getCommits().addAll(commitPersistence.elementHistory(projectId, elementId, commitIds));
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Copy link

Quality Gate Failed Quality Gate failed

Failed conditions
1 Security Hotspot
0.0% Coverage on New Code (required ≥ 80%)
7.8% Duplication on New Code (required ≤ 3%)
C Reliability Rating on New Code (required ≥ A)

See analysis details on SonarCloud

idea Catch issues before they fail your Quality Gate with our IDE extension SonarLint SonarLint

Doris Lam added 3 commits February 13, 2024 13:53
…vice doesn't need to pass in commitIds (this is so it's not getting json just to get ids as getting entire commit json is expensive for federated persistence)
Branch b = new Branch(getProject(projectId), branchId, false);
branchRepo.save(b);
return b;
RefJson b = new RefJson();
Copy link
Collaborator

@dlamoris dlamoris Mar 18, 2024

Choose a reason for hiding this comment

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

before was only creating global branch, check what side effects using persistence.save have
theoretically it should never create here

Copy link
Collaborator

Choose a reason for hiding this comment

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

fixed in #260

@@ -1,36 +0,0 @@
package org.openmbee.mms.permissions.delegation;
Copy link
Collaborator

Choose a reason for hiding this comment

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

moved into federated module

@@ -71,6 +74,17 @@ public boolean hasPermission(String user, Set<String> groups, String privilege)
return false;
}

@Override
public boolean hasGroupPermissions(String group, String privilege) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

this is actually not used anywhere, just here to fulfill an interface

@@ -60,7 +60,7 @@ protected Pair<Group, Role> getGroupAndRole(PermissionUpdateRequest.Permission p
Optional<Group> group = getGroupRepo().findByName(p.getName());
Optional<Role> role = getRoleRepo().findByName(p.getRole());
if (!role.isPresent()) {
return Pair.of(group.orElse(null), null);
return null;
Copy link
Collaborator

Choose a reason for hiding this comment

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

line 67 should keep the returned instance

Copy link
Collaborator

@dlamoris dlamoris Mar 26, 2024

Choose a reason for hiding this comment

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

fixed in #260

fix argument in DefaultPermissionService.setProjectInherit
private FederatedPermissionsUpdateResponseBuilder groupsBuilder = new FederatedPermissionsUpdateResponseBuilder();

@Override
public FederatedPermissionUpdatesResponseBuilder setInherit(Boolean inherit) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

setInherit, setPublic, insert functions are never called on this class, only on parent class, can be removed

Copy link
Collaborator

Choose a reason for hiding this comment

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

fixed in #260

@@ -0,0 +1,99 @@
package org.openmbee.mms.federatedpersistence.permissions;
Copy link
Collaborator

Choose a reason for hiding this comment

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

these were functions from PermissionsUpdateResponseBuilder that uses the DB classes

@@ -50,93 +44,6 @@ public PermissionUpdateResponseBuilder insert(PermissionUpdateResponse updateUse
return this;
}

public void insertPermissionUpdates_OrgUserPerm(PermissionUpdateResponse.Action action, Collection<OrgUserPerm> perms) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

these are moved to federated module

@@ -88,6 +85,17 @@ public boolean hasPermission(String user, Set<String> groups, String privilege)
return false;
}

@Override
public boolean hasGroupPermissions(String group, String privilege) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

never called, just implements interface

@@ -120,15 +128,28 @@ public boolean setInherit(boolean isInherit) {
return false;
}

@Override
public PermissionResponse getInherit() {
Copy link
Collaborator

Choose a reason for hiding this comment

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

never called

@dlamoris dlamoris mentioned this pull request Mar 31, 2024
Copy link

sonarqubecloud bot commented Apr 9, 2024

Quality Gate Failed Quality Gate failed

Failed conditions
1 Security Hotspot
50.6% Coverage on New Code (required ≥ 80%)
7.2% Duplication on New Code (required ≤ 3%)
C Reliability Rating on New Code (required ≥ A)

See analysis details on SonarCloud

Catch issues before they fail your Quality Gate with our IDE extension SonarLint

@dlamoris dlamoris merged commit 7de6dd0 into develop Apr 9, 2024
3 of 5 checks passed
@dlamoris dlamoris deleted the feature/persistence_refactor branch August 27, 2024 16:37
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.

3 participants