Feature/commit refactor by HuiJun · Pull Request #135 · Open-MBEE/exec-mms

HuiJun · 2021-01-22T01:38:52Z

No description provided.

…iated commits

dlamoris

initial comments

dlamoris · 2021-01-22T02:18:45Z

elastic/src/main/java/org/openmbee/mms/elastic/CommitElasticDAOImpl.java


-    public Optional<CommitJson> findById(String docId) {
-        return this.findById(getIndex(), docId);
+    public void index(BaseJson json, String commitId) {


i don't think this is needed - the caller should just give the right commitJson to index and not have to know about commitId overrides

Ok, removed

dlamoris · 2021-01-22T02:19:57Z

elastic/src/main/java/org/openmbee/mms/elastic/CommitElasticDAOImpl.java


    public void indexAll(Collection<? extends BaseJson> jsons) {
-        this.indexAll(getIndex(), jsons);
+        this.indexAll(getIndex(), associateCommits(jsons));


this should index commits 'as is' (see below for more), it shouldn't assume that indexing a collection of commit json means they're all related
also, the signature of this and index should probably change to CommitJson instead of BaseJson

dlamoris · 2021-01-22T02:23:12Z

elastic/src/main/java/org/openmbee/mms/elastic/CommitElasticDAOImpl.java

    }

    public void index(BaseJson json) {
+        if (json.get(CommitJson.COMMITID).toString().isEmpty()) {


I think what should happen here (and in indexAll) is the breaking up of a single commit json into several if its arrays exceeds some threshold - so the caller can give a huge object, but because of the elasticsearch limits, only the elastic impl knows to break it up (similar to how the get knows to munge commit objects together and can return a huge object to the caller, the caller doesn't need to know about munging)

At the same time, if the caller already broke up the commits, this would still work as expected

Done, but it might be done in a different way

dlamoris · 2021-01-22T02:26:29Z

elastic/src/main/java/org/openmbee/mms/elastic/CommitElasticDAOImpl.java

+    }
+
+    public void deleteById(String commitId) {
+        this.deleteById(getIndex(), commitId);


this should delete all docs that have commitId, right now it treats commitId as the docId

dlamoris · 2021-01-22T02:26:55Z

elastic/src/main/java/org/openmbee/mms/elastic/CommitElasticDAOImpl.java

-    public boolean existsById(String docId) {
-        return this.existsById(getIndex(), docId);
+    public boolean existsById(String commitId) {
+        return this.existsById(getIndex(), commitId);


same as delete ?

dlamoris · 2021-01-22T02:37:50Z

crud/src/main/java/org/openmbee/mms/crud/services/DefaultNodeService.java


+        String commitId = params.get("commitId");
+        if (commitId != null && !commitId.isEmpty()) {
+            info.getCommitJson().setCommitId(commitId);


this should happen during the createCommit in the previous call, otherwise the elements getting updated will have the wrong commitId

dlamoris · 2021-01-22T02:40:56Z

crud/src/main/java/org/openmbee/mms/crud/services/NodeOperation.java


    public void initCommitJson(CommitJson cmjs, Instant now) {
        cmjs.setId(UUID.randomUUID().toString());
+        cmjs.setCommitId(UUID.randomUUID().toString());


this should check if commitId is not set first, if it's not set, it should just be same as the id so it's still consistent with any existing paradigm

crud/src/main/java/org/openmbee/mms/crud/services/NodeOperation.java

HuiJun · 2021-01-25T18:31:36Z

elastic/src/main/java/org/openmbee/mms/elastic/CommitElasticDAOImpl.java

+                broken.add(currentCommitCopy);
+
+            }
+            this.indexAll(broken);


Just fyi, this adds a property called "action" into the commit objects. We could remove it if it's a problem.

dlamoris · 2021-01-26T20:45:22Z

crud/src/main/java/org/openmbee/mms/crud/services/DefaultNodeService.java

-                commit.setComment(cmjs.getComment());
-
-                this.commitRepository.save(commit);
+                Optional<Commit> existing = this.commitRepository.findByCommitId(cmjs.getCommitId());


cmjs.getId()

dlamoris · 2021-01-26T20:46:15Z

crud/src/main/java/org/openmbee/mms/crud/services/DefaultNodeService.java

        cmjs.setProjectId(projectId);
+
+        if (commitId != null && !commitId.isEmpty()) {
+            cmjs.setId(commitId);


there's somewhere else (initCommit)? that needs to be updated to check for existing id and generate docId

dlamoris · 2021-01-26T20:47:31Z

crud/src/main/java/org/openmbee/mms/crud/services/NodeOperation.java


        n.setDocId(e.getDocId());
-        n.setLastCommit(cmjs.getId());
+        n.setLastCommit(cmjs.getCommitId());


dlamoris · 2021-01-26T20:50:11Z

elastic/src/main/java/org/openmbee/mms/elastic/CommitElasticDAOImpl.java

+                        case "deleted":
+                            currentCommitCopy.getDeleted().add(action);
+                            break;
+                    }


can remove action here?

dlamoris · 2021-01-26T20:57:55Z

elastic/src/main/java/org/openmbee/mms/elastic/CommitElasticDAOImpl.java

+    private List<CommitJson> getDocs(String commitId) {
+        try {
+            QueryBuilder commitQuery = QueryBuilders.boolQuery()
+                .should(QueryBuilders.termQuery(CommitJson.ID, commitId))


this is better as a filter instead of should

dlamoris · 2021-01-26T21:05:51Z

elastic/src/main/java/org/openmbee/mms/elastic/CommitElasticDAOImpl.java

 public class CommitElasticDAOImpl extends BaseElasticDAOImpl<CommitJson> implements CommitIndexDAO {

+    @Value("${elasticsearch.limit.index}")
+    int indexLimit;


should we be using the same limit for indexing vs splitting up commit? the commit can be a lot bigger than whatever we set the index limit to (currently used in bulk processor) (this is already injected in the BaseDao as bulkLimit)

dlamoris · 2021-01-26T21:14:38Z

elastic/src/main/java/org/openmbee/mms/elastic/CommitElasticDAOImpl.java

+                partial.setId("");
+            }
+
+            if (partial.getAdded().isEmpty() && raw.getAdded() != null) {


I don't think the first check should be here? this would only add addition data if partial is empty but we want to add them regardless

dlamoris · 2021-01-26T21:24:37Z

elastic/src/main/java/org/openmbee/mms/elastic/CommitElasticDAOImpl.java

+            if (partial.getId() == null) {
+                partial.setId("");
+            }
+            if (partial.getId() == null) {


dlamoris · 2021-01-26T21:24:58Z

elastic/src/main/java/org/openmbee/mms/elastic/CommitElasticDAOImpl.java

+            if (partial.getComment().isEmpty() && raw.getComment() != null) {
+                partial.setComment(raw.getComment());
+            }
+            if (partial.getId().isEmpty() && raw.getCommitId() != null) {


dlamoris · 2021-01-26T21:25:41Z

crud/src/main/java/org/openmbee/mms/crud/controllers/elements/ElementsController.java

        throw new BadRequestException(response.addMessage("Empty"));
    }

+    @PostMapping(value = "/stream", consumes = MediaType.APPLICATION_JSON_VALUE)


can comment this function out for now so it doesn't show up in generated swagger

dlamoris · 2021-01-26T22:11:44Z

elastic/src/main/java/org/openmbee/mms/elastic/CommitElasticDAOImpl.java

            }
-            return commits;
+            ArrayList<CommitJson> result = new ArrayList<>();
+            for (String key : rawCommits.keySet()) {


does using keySet retain ordering?
also because of the query, the commit objects returned wouldn't be the entire object if it was split - we used to just return the commit id and some metadata for element history, maybe we can just do that again

dlamoris · 2021-01-26T22:28:45Z

example/crud.postman_collection.json

 			},
 			"response": []
 		},
-		{


revert deletion

dlamoris · 2021-01-26T22:51:40Z

elastic/src/main/resources/elastic_mappings/commit.json

            "_projectId": {
                "type": "keyword"
            },
-            "_docId": {


dlamoris · 2021-01-26T22:52:10Z

json/src/main/java/org/openmbee/mms/json/CommitJson.java

    public static final String SOURCE = "source";

+    public static CommitJson copy(CommitJson original) {
+        CommitJson copy = new CommitJson();


copy comment

…d as parameter

add creator,created,refid,projectid to munge commits

…/mms into feature/commitRefactor

HuiJun added 2 commits January 20, 2021 11:33

Refactor to use commitId instead of docId and adding munging of assoc…

508f9b4

…iated commits

Add commitId support for existing commits

60ebac5

HuiJun requested a review from dlamoris January 22, 2021 01:39

dlamoris requested changes Jan 22, 2021

View reviewed changes

Lam, Doris T (319E) and others added 2 commits January 22, 2021 17:36

use bulk processor for elastic script update on branch create

d2491c7

Break up commits by size internally and move commit id setting

fb1970f

HuiJun commented Jan 25, 2021

View reviewed changes

HuiJun requested a review from dlamoris January 25, 2021 19:12

HuiJun added 4 commits January 25, 2021 11:35

Assign id to commit id, if not set

989188e

Increase delay for commit indexing

aa9fa37

Increase delay for cameo tests

c7bdbb8

Refactor to use commit object's id as commit id

bff1da4

dlamoris requested changes Jan 26, 2021

View reviewed changes

Remove action attribute fix more commit id references

9d507b9

dlamoris reviewed Jan 26, 2021

View reviewed changes

example/crud.postman_collection.json

},

"response": []

},

{

Copy link

Collaborator

dlamoris Jan 26, 2021

Choose a reason for hiding this comment

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

revert deletion

dlamoris reviewed Jan 26, 2021

View reviewed changes

elastic/src/main/resources/elastic_mappings/commit.json

"_projectId": {

"type": "keyword"

},

"_docId": {

Copy link

Collaborator

dlamoris Jan 26, 2021

Choose a reason for hiding this comment

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

revert

dlamoris reviewed Jan 26, 2021

View reviewed changes

HuiJun and others added 12 commits January 27, 2021 09:41

Optimize element history and change fields to text instead of varchar

35538b5

Revert commitId elastic mapping

9aad069

Change length of fields and revert deleted test

85dae16

Add parameter support for projects endpoint

5e60981

Implement get project by org id in project repository and define orgI…

68cc229

…d as parameter

add test for get project by org,

2126179

add creator,created,refid,projectid to munge commits

add test for getting nonexisting commit, fix optional return

01c4ecd

Clean up copy method

8772ad3

Merge branch 'feature/commitRefactor' of https://github.com/Open-MBEE…

fce1b10

…/mms into feature/commitRefactor

fill in ref and project id for ref commits

54b27f2

Simplify existsById method for commits

1015cd1

Merge branch 'feature/commitRefactor' of https://github.com/Open-MBEE…

fb26f76

…/mms into feature/commitRefactor

dlamoris approved these changes Jan 28, 2021

View reviewed changes

dlamoris merged commit 060ef8c into develop Jan 28, 2021

dlamoris deleted the feature/commitRefactor branch January 28, 2021 19:54

Uh oh!

Conversation

HuiJun commented Jan 22, 2021

Uh oh!

dlamoris left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dlamoris Jan 26, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

dlamoris Jan 26, 2021 •

edited

Loading