MLE-25586 add trans closure to node client #1045
Conversation
Add transitive closure function to node client. Function body generated by Optic Code generator. Manual changes to base, and creation of tests. Increase timeout on an SSL v1.2 test, passes most of the time now Skip a broken test, will fix later.
stevebio
requested review from
BillFarber,
anu3990 and
rjrudin
as code owners
|
Copyright Validation Results ⏭️ Skipped (Excluded) Files
✅ Valid Files
✅ All files have valid copyright headers! |
There was a problem hiding this comment.
Pull request overview
This PR adds transitive closure functionality to the Node.js client library. The implementation includes the generated function code, test cases, and supporting test data files. Two minor test adjustments were also made: increasing a timeout for an SSL test and skipping a failing test.
Key changes:
- Added
transitiveClosure()method to the plan builder API with options for configuring path length constraints - Created comprehensive test suite covering various transitive closure scenarios including basic patterns, min/max length constraints, and joins
- Added test data files with semantic triples representing parent-child relationships for testing
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| test-basic/transitive-closure.js | New test file with 5 test cases covering different transitive closure scenarios |
| test-basic/ssl-min-allow-tls-test.js | Increased timeout from 10000ms to 12000ms |
| test-basic/service-caller.js | Skipped failing test with explanatory comment |
| test-app/src/main/ml-data/optic/transitive-closure/transClosureTripleSet.xml | Test data defining semantic triples for parent-child relationships |
| test-app/src/main/ml-data/optic/transitive-closure/permissions.properties | Permissions configuration for test data |
| test-app/src/main/ml-data/optic/transitive-closure/collections.properties | Collections configuration for test data |
| lib/plan-builder-generated.js | Generated code adding PlanTransitiveClosureOptions class and transitiveClosure method |
| lib/plan-builder-base.js | Validation logic for transitive closure options |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| } | ||
| return true; | ||
| case 'PlanTransitiveClosureOptions': | ||
| const planTransitiveClosureOptionsSet = new Set(['minLength', 'min-length', 'maxLength', 'max-length']); |
There was a problem hiding this comment.
The options set includes both camelCase and kebab-case versions of the same options. This dual naming convention could cause confusion for API consumers. Consider documenting which format is preferred or standardizing on one naming convention.
There was a problem hiding this comment.
This is consistent with previous impl
rjrudin
left a comment
rjrudin
left a comment
There was a problem hiding this comment.
About to test locally, will respond back.
| }); | ||
|
|
||
| it('postOfUrlencodedForDocumentArray1 endpoint', function(done) { | ||
| // errors all the time now, should fix. |
There was a problem hiding this comment.
This is passing consistently on Jenkins - https://ml-clt-jenkins.progress.com/job/devexp/job/Node-Client/job/Node-client-api/job/develop/160/ - so maybe just a local issue? Does it fail when you run all of "test-basic"?
|
|
||
| describe('document write and read using min tls', function () { | ||
| this.timeout(10000); | ||
| this.timeout(12000); |
There was a problem hiding this comment.
Was this failing for you locally too? It's another test that I don't think has failed on Jenkins in quite a while. Of course, 10s is already totally arbitrary, so changing it to 12s doesn't really hurt.
There was a problem hiding this comment.
Yes the timing for the error ssl v1.2 error test was like 11000ms for me. so 120000 timeout fixed locally. But totally environmental and not even close to a good solution. I could bump to 200000.
|
|
||
| it('with simple pattern full transitive closure', function (done) { | ||
| execPlan( | ||
| p.fromTriples([ |
There was a problem hiding this comment.
This is great, just getting a test case in place is like 98% of the value of the PRs for these new Optic functions, as now we have a way to test / demo it. I'll try it out locally.
| @@ -0,0 +1,141 @@ | |||
| /* | |||
There was a problem hiding this comment.
A few of the imports in here are flagged by VS Code as unused (I guess the nom run lint doesn't complain on unused imports yet) - should, assert, and restWriterConnection. Remove those and then merge away!
Add transitive closure function to node client. Function body generated by Optic Code generator. Manual changes to base, and creation of tests. Increase timeout on an SSL v1.2 test, passes most of the time now Skip a broken test, will fix later.