Q20-15 - story/Q20-15 #33

ShivaniYerram · 2023-05-25T21:15:06Z

Guidelines for Pull Requests

If you haven't yet read our code review guidelines, please do so, You can find them here.

Please confirm the following by adding an x for each item (turn [ ] into [x]).

I have removed all code style changes that are not necessary (e.g. changing blanks across the whole file that don’t need to be changed, adding empty lines in parts other than your own code)
I am not making any changes to files that don’t have any effect (e.g. imports added that don’t need to be added)
I do not have any sysout statements in my code or commented out code that isn’t needed anymore
I am not reformatting any files in the wrong format or without cause.
I am not changing file encoding or line endings to something else than UTF-8, LF
My pull request does not show an insane amount of files being changed although my ticket only requires a few files being changed
I have added Javadoc/documentation where appropriate
I have added test cases where appropriate
I have explained any part of my code/implementation decisions that is not be self-explanatory

Please provide a brief description of your ticket

(you can copy the ticket if it hasn't changed)

The endpoint needs to accept a list of mappings and patterns (pattern1 -> mapping1, pattern2 -> mapping2, ...). Quadriga would then create a new "Collection Mapping" that transforms all the graphs that are matched by one of the patterns with the corresponding pattern. Basically, new groups of triples are being created (in addition to the default mapping one).

Anything else the reviewer needs to know?

... describe here ...

…ph node, only check for type if the concept has one

…nd query by 'equal_to' field otherwise

…for a pattern match

…rkApiController.

…ollers.

ShivaniYerram · 2024-02-28T22:47:25Z

quadriga/src/main/java/edu/asu/diging/quadriga/core/service/impl/ConceptFinderImpl.java

+            return uri + "/";
+        } else if (!containsHttps) {
+            uri = uri.replaceFirst("http", "https");
+            uri = uri.endsWith("/") ? uri.substring(0, uri.length() - 1) : uri;


Can check whether we need a https with "/" or without "/", once Q20-16 is merged.

…ed for some remaining changes.

jdamerow · 2024-03-28T15:10:16Z

quadriga/pom.xml

@@ -45,11 +45,13 @@
        <neo4j.url>http://neo4j:quadriga@localhost:7474/</neo4j.url>
        <neo4j.database>quadriga</neo4j.database>

-        <citesphere.base.url></citesphere.base.url>
+        <citesphere.base.url>/</citesphere.base.url>


this should not be here

unaddressed

jdamerow · 2024-03-28T15:11:32Z

quadriga/src/main/java/edu/asu/diging/quadriga/api/v1/MapGraphToTripleController.java

+        if (eventGraphs == null) {
+            return new ResponseEntity<>(HttpStatus.NOT_FOUND);
+        }
+        System.out.println(patternMappingList);        


jdamerow · 2024-03-28T15:12:54Z

quadriga/src/main/java/edu/asu/diging/quadriga/api/v1/MapGraphToTripleController.java

+    public ResponseEntity<List<JobPatternInfo>> mapPatternToTriples(@PathVariable String collectionId,
+            @RequestBody List<PatternMapping> patternMappingList) {
+
+        List<EventGraph> eventGraphs = eventGraphService.getEventGraphs(new ObjectId(collectionId));


these can be a lot of graphs. You don't want to load them all into memory. I don't think you want this done here.

jdamerow · 2024-03-28T15:15:23Z

quadriga/src/main/java/edu/asu/diging/quadriga/api/v1/MapGraphToTripleController.java

+
+        }
+
+        return new ResponseEntity<>(HttpStatus.NOT_FOUND);


when would that be the case? And why does it return "not found"? what is not found?

jdamerow · 2024-03-28T15:15:33Z

quadriga/src/main/java/edu/asu/diging/quadriga/api/v1/model/JobPatternInfo.java

+ * @param track is the URI of the status of the job.
+ * @param explore is the URI of the collection whose graphs are being mapped with the input patterns.
+ */
+


jdamerow · 2024-03-28T15:22:18Z

quadriga/src/main/java/edu/asu/diging/quadriga/core/data/IdGenerator.java

@@ -0,0 +1,33 @@
+package edu.asu.diging.quadriga.core.data;


jdamerow · 2024-03-28T15:24:58Z

quadriga/src/main/java/edu/asu/diging/quadriga/core/model/jobs/JobStatus.java

@@ -0,0 +1,9 @@
+package edu.asu.diging.quadriga.core.model.jobs;
+
+public enum JobStatus {


jdamerow · 2024-03-28T15:32:31Z

quadriga/src/main/java/edu/asu/diging/quadriga/core/service/impl/AsyncPatternProcessorImpl.java

+                    jobRepository.save(job);
+                }
+            } else {
+                mappedTripleGroup = mappedTripleGroupService.get(collectionId, MappedTripleType.CUSTOM_MAPPING);


That does not make sense. There can be multiple custom mappings for a collection, each in their own custom mapping triple group. Also, a method called get should get an element, not create it (I missed that before).

jdamerow · 2024-03-28T15:40:03Z

quadriga/src/main/java/edu/asu/diging/quadriga/core/service/impl/AsyncPatternProcessorImpl.java

+        }
+
+        for (EventGraph network : networks) {
+            List<Graph> extractedGraphs = patternFinder.findGraphsWithPattern(patternMapping.getMetadata(), patternRoot,


is that method only finding graphs or also mapping them? It seems like it's also mapping them, in which case the method name is incorrect.

jdamerow · 2024-03-28T15:43:26Z

quadriga/src/test/java/edu/asu/diging/quadriga/api/v1/AddNetworkApiControllerTest.java

+    }
+
+    @Test
+    void testProcessJson_Success() throws NodeNotFoundException, InvalidObjectIdException, CollectionNotFoundException {


please adhere to our test naming guidelines

jdamerow · 2024-04-30T17:27:37Z

quadriga/pom.xml

@@ -45,11 +45,13 @@
        <neo4j.url>http://neo4j:quadriga@localhost:7474/</neo4j.url>
        <neo4j.database>quadriga</neo4j.database>

-        <citesphere.base.url></citesphere.base.url>
+        <citesphere.base.url>/</citesphere.base.url>


unaddressed

jdamerow · 2024-04-30T17:39:47Z

quadriga/src/main/java/edu/asu/diging/quadriga/api/v1/MapGraphToTripleController.java

+        List<EventGraph> eventGraphs = eventGraphService.getEventGraphsByCollectionId(new ObjectId(collectionId), PageRequest.of(pageNumber, pageSize));
+
+        if (eventGraphs == null) {
+            return new ResponseEntity<>(HttpStatus.NOT_FOUND);


I think the logic should rather be to check if the collection exists, not the if event graphs exists. it's fine (even if it doesn't make much sense) to try to run mappings on 0 event graphs. An invalid collection id, however, should return a not found.

jdamerow · 2024-04-30T17:40:14Z

quadriga/src/main/java/edu/asu/diging/quadriga/api/v1/MapGraphToTripleController.java

+
+        MappedTripleGroup mappedTripleGroup = new MappedTripleGroup();
+        mappedTripleGroup.set_id(new ObjectId());
+        mappedTripleGroup.setCollectionId(new ObjectId(collectionId));


this should not be part of the controller, that's business logic.

jdamerow · 2024-04-30T17:41:02Z

quadriga/src/main/java/edu/asu/diging/quadriga/api/v1/MapGraphToTripleController.java

+
+        String jobId = jobManager.createJob(collectionId, mappedTripleGroup.get_id().toString(), eventGraphs.size());
+
+        List<JobPatternInfo> jobInfos = mapGraphToTriple.mapPatterns(collectionId, jobId, eventGraphs, patternMappingList);


same with these two lines. The controller should just call one method that takes care of the rest.

jdamerow · 2024-04-30T17:41:08Z

quadriga/src/main/java/edu/asu/diging/quadriga/api/v1/model/JobPatternInfo.java

+ * @param track is the URI of the status of the job.
+ * @param explore is the URI of the collection whose graphs are being mapped with the input patterns.
+ */
+


jdamerow · 2024-04-30T17:41:51Z

quadriga/src/main/java/edu/asu/diging/quadriga/api/v1/service/impl/MapGraphToTripleImpl.java

+import edu.asu.diging.quadriga.core.service.AsyncPatternProcessor;
+
+@Service
+public class MapGraphToTripleImpl implements MapGraphToTriple{


bad class name. class names should be nouns.

jdamerow · 2024-04-30T17:49:36Z

quadriga/src/main/java/edu/asu/diging/quadriga/api/v1/service/impl/MapGraphToTripleImpl.java

+            jobInfos.add(jobInfo);
+        }
+
+        return jobInfos;


ah, right. I think I would structure the response differently though. multiple job infos with the same job id indicates the ids could be different. Instead, i would have one job object with an id that then has a list of mapping infos.

jdamerow · 2024-04-30T17:51:20Z

quadriga/src/main/java/edu/asu/diging/quadriga/core/data/IdGenerator.java

@@ -0,0 +1,33 @@
+package edu.asu.diging.quadriga.core.data;


jdamerow · 2024-04-30T17:57:05Z

quadriga/src/main/java/edu/asu/diging/quadriga/core/service/impl/AsyncPatternProcessorImpl.java

+                    jobRepository.save(job);
+                }
+            } else {
+                mappedTripleGroup = mappedTripleGroupService.getMappedTripleGroupIfExistsOrAdd(collectionId, MappedTripleType.CUSTOM_MAPPING);


this does not seem right, wouldn't that always return the first custom mapping it finds?

jdamerow · 2024-04-30T18:01:16Z

quadriga/src/main/java/edu/asu/diging/quadriga/core/service/impl/AsyncPatternProcessorImpl.java

+                    network);
+            for (Graph extractedGraph : extractedGraphs) {
+                try {
+                    mappedTripleService.storeMappedGraph(extractedGraph, mappedTripleGroup);


there needs to be some documentation about this somewhere, or even better, default mapping should not be called default mapping.

MaulikLim and others added 30 commits June 15, 2022 16:28

[Q20-15] - Model for find pattern api request

b3a7d40

[Q20-15] - Classes for holding the pattern graph

eac4100

[Q20-15] - Building pattern graph

77a175b

[Q20-15] - Finding patterns in the graph

7aeff7a

[Q20-15] - Created models for holding response from conceptpower

2297cc0

[Q20-15] - Parent conceptpower response class

5d940c8

[Q20-15] - Conceptpower Connector Interface

c1d622b

[Q20-15] - Conceptpower connector

ce2e3fa

[Q20-15] - extracting subgraphs that match the pattern

274d9b4

[Q20-15] - mapping sub-graphs to triples

9ccc289

[Q20-15] - match pattern interpretation with alternative ids of a gra…

26e079d

…ph node, only check for type if the concept has one

[Q20-15] - create mapped triple group if not found

3c0fff9

[Q20-15] - get by id when interpretation contains term 'digitalhps' a…

47ea50c

…nd query by 'equal_to' field otherwise

[Q20-15] - iterate over all event graphs for a collection and search …

8eaaf88

…for a pattern match

[Q20-15] - code refactoring

ee62a04

[Q20-15] - asynchronous processing for patterns

bab0acb

[Q20-15] - restructured the code for job creation and management

cfcd8af

[Q20-15] - correcting minor mistake

03ebd67

[Q20-15] - bug fixes - error handling

3b28ae4

[Q20-15] - minor corrections

c0852d9

[Q20-15] - code cleanup, javadocs, renamed classes

452c08e

[Q20-15] - changing response details for map triples API

7211aa9

[Q20-15] - configuring quadriga uris for response

5abf144

[Q20-15] - respository query correction

6200570

[Q20-15] Added mapGraphToPattern controller

f461cec

[Q20-15]Tested response from job api.

c4e0d03

[Q20-15] Added test cases for MapGraphToTripleController and AddNetwo…

481cee5

…rkApiController.

[Q20-15] Added test cases for MapGraphToTripleController and AddNetwo…

362cb65

…rkApiController.

[Q20-15] Added testcases for MapGraphToTriple and AddNetworkApi contr…

5a22df9

…ollers.

[Q20-15] Added Test cases for controllers.

392d85c

ShivaniYerram commented Feb 28, 2024

View reviewed changes

ShivaniYerram added 2 commits February 28, 2024 16:02

[Q20-15] Addressed review comments.

4d5f37f

[Q20-15] Addressed review comments, should wait for Q20-16 to be merg…

e462496

…ed for some remaining changes.

ShivaniYerram reopened this Feb 28, 2024

jdamerow requested changes Mar 28, 2024

View reviewed changes

jdamerow closed this Mar 28, 2024

ShivaniYerram added 2 commits April 8, 2024 15:52

[Q20-15] Addressed review comments.

53cb294

[Q20-15] Addressed review comments.

e185877

ShivaniYerram reopened this Apr 10, 2024

[Q20-15] Resolved code factor: Placing overloaded methods together.

e249731

jdamerow requested changes Apr 30, 2024

View reviewed changes

jdamerow closed this Apr 30, 2024

pooja-thalur added 5 commits September 6, 2024 16:38

[Q20-15] removed pom.xml change

6c7a5fe

[Q20-15] config changes

86a25d3

[Q20-15] moving business logic from controller

19cdda1

[Q20-15] addressed review comments

814fb46

[Q20-15] review comments addressed and testcases issue fixed

83e2a0e

pooja-thalur reopened this Sep 18, 2024

pooja-thalur closed this Sep 18, 2024

[Q20-15] codefactor issue

f71b945

pooja-thalur reopened this Sep 18, 2024

pooja-thalur added 4 commits September 18, 2024 11:35

[Q20-15] addressed review comments

c26eb0f

[Q20-15] removed spaces and empty lines

519fcf6

[Q20-15] fixing codefactor issue

06d4998

[Q20-15] reverting the code factor issue fix

5bacdcd

pooja-thalur marked this pull request as draft September 18, 2024 19:05

pooja-thalur closed this Nov 12, 2024

pooja-thalur reopened this Dec 4, 2024

Merge branch 'develop' into story/Q20-15

f28fc45

pooja-thalur marked this pull request as ready for review December 16, 2024 15:57

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Q20-15 - story/Q20-15 #33

Q20-15 - story/Q20-15 #33

ShivaniYerram commented May 25, 2023

ShivaniYerram Feb 28, 2024

jdamerow Mar 28, 2024

jdamerow Apr 30, 2024

jdamerow Mar 28, 2024

jdamerow Mar 28, 2024

jdamerow Mar 28, 2024

jdamerow Mar 28, 2024

jdamerow Apr 30, 2024

jdamerow Mar 28, 2024

jdamerow Mar 28, 2024

jdamerow Mar 28, 2024

jdamerow Mar 28, 2024

jdamerow Mar 28, 2024

jdamerow Apr 30, 2024

jdamerow Apr 30, 2024

jdamerow Apr 30, 2024

jdamerow Apr 30, 2024

jdamerow Apr 30, 2024

jdamerow Apr 30, 2024

jdamerow Apr 30, 2024

jdamerow Apr 30, 2024

jdamerow Apr 30, 2024

jdamerow Apr 30, 2024

		@@ -0,0 +1,9 @@
		package edu.asu.diging.quadriga.core.model.jobs;

		public enum JobStatus {


		String jobId = jobManager.createJob(collectionId, mappedTripleGroup.get_id().toString(), eventGraphs.size());

		List<JobPatternInfo> jobInfos = mapGraphToTriple.mapPatterns(collectionId, jobId, eventGraphs, patternMappingList);

Q20-15 - story/Q20-15 #33

Are you sure you want to change the base?

Q20-15 - story/Q20-15 #33

Conversation

ShivaniYerram commented May 25, 2023

Guidelines for Pull Requests

Please provide a brief description of your ticket

Anything else the reviewer needs to know?

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

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