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

#553 #595

Merged
merged 24 commits into from
Dec 12, 2017
Merged

#553 #595

merged 24 commits into from
Dec 12, 2017

Conversation

paxtonhare
Copy link
Contributor

marklogic/datahub-central#1143

@paxtonhare paxtonhare requested a review from dmcassel December 1, 2017 14:55
@@ -19,6 +19,19 @@ module namespace es-wrapper = "http://marklogic.com/data-hub/es-wrapper";

declare option xdmp:mapping "false";

declare function es-wrapper:search-options-generate($model)
{
xdmp:eval('
Copy link
Contributor

Choose a reason for hiding this comment

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

why is this an eval instead of just calling es:search-options-generate()?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

this is part of the support for ML8. If this module didn't have the eval then the REST API would barf on the static check on ML8 when the code is deployed. If we drop ML8 support then this can go away.

@@ -187,84 +75,39 @@ declare function hent:wrap-duplicates(
$item)
};

declare function hent:dump-search-options($entities as json:array)
declare %private function hent:fix-options($nodes as node()*)
Copy link
Contributor

Choose a reason for hiding this comment

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

I can see that this function replaces "es:" with "*:" in text nodes, but it's not clear why that's a useful thing. Please add a comment.

Copy link
Contributor

Choose a reason for hiding this comment

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

Just saw the comment further down about entity services issue #359. Okay.

map:delete($o, $x)
let $_ :=
for $idx in json:array-values(map:get($o, "range-path-index"))
let $_ := xdmp:log(("idx:", $idx))
Copy link
Contributor

Choose a reason for hiding this comment

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

is it still useful to have this xdmp:log statement?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

no. it should go away

allCombos((codeFormat, dataFormat, flowType) -> {
allCombos((codeFormat, dataFormat, flowType, useEs) -> {
if (useEs) {
return;
Copy link
Contributor

Choose a reason for hiding this comment

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

we've got this return, but the code below also has useEs ? "-es" : "". Unclear why we'd return here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

because we don't want to run ES tests in this case. and the useEs ? "-es" : "" was me going copy/paste crazy. I will remove the useEs ? "-es" : "" to make it a bit more clear. maybe a comment too :)

allCombos((codeFormat, dataFormat, flowType) -> {
allCombos((codeFormat, dataFormat, flowType, useEs) -> {
if (useEs) {
return;
Copy link
Contributor

Choose a reason for hiding this comment

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

as above, unclear why we're returning when we're testing for useEs below.

String flowName = getFlowName(prefix, codeFormat, dataFormat, flowType);
if (flowType.equals(FlowType.INPUT)) {
String flowName = getFlowName(prefix, codeFormat, dataFormat, flowType, useEs);
if (flowType.equals(FlowType.INPUT) && useEs) {
Copy link
Contributor

Choose a reason for hiding this comment

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

are there input flows that don't use ES? those would go to the else clause, which tests harmonize flows.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

good catch.

tests.add(DynamicTest.dynamicTest(flowName, () -> {
// clear out the previous flows from above
FileUtils.deleteDirectory(entityDir.resolve("input").toFile());
FileUtils.deleteDirectory(entityDir.resolve("harmonize").toFile());

createFlow(prefix, codeFormat, dataFormat, flowType, (codeFormat1, dataFormat1, flowType1, srcDir, flowDir) -> {
copyFile(srcDir + "content-syntax-error." + codeFormat.toString(), flowDir.resolve("content." + codeFormat.toString()));
createFlow(prefix, codeFormat, dataFormat, flowType, useEs, (codeFormat1, dataFormat1, flowType1, srcDir, flowDir, useEs1) -> {
Copy link
Contributor

Choose a reason for hiding this comment

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

I see that codeFormat1 isn't a new variable, but it is pretty opaque.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

suggestions?

Copy link
Contributor

Choose a reason for hiding this comment

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

I've got nothing. leave it

assertEquals(finalCounts.finalCount, getFinalDocCount());
assertEquals(finalCounts.tracingCount, getTracingDocCount());
assertEquals(finalCounts.jobCount, getJobDocCount());
int stagingCount = getStagingDocCount();
Copy link
Contributor

Choose a reason for hiding this comment

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

curious about this change -- it's fine, just curious

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it allowed me to put a conditional breakpoint when finalCount != finalCounts.finalCount. It was merely there for my debugging purposes.

Copy link
Contributor

Choose a reason for hiding this comment

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

cool

@@ -799,7 +857,7 @@ else if (prefix.equals("1x-legacy")) {
if (dataFormat.equals(DataFormat.JSON)) {
String expected = getResource("e2e-test/" + filename + "." + dataFormat.toString());
String actual = stagingDocMgr.read("/input" + fileSuffix + "." + dataFormat.toString()).next().getContent(new StringHandle()).get();
assertJsonEqual(expected, actual, false);
assertJsonEqual(expected, actual, false);
Copy link
Contributor

Choose a reason for hiding this comment

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

looks like an accidental space here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

are you talking about my brain? lol. yes. this is.

@@ -0,0 +1 @@
{"path-namespace":[{"prefix":"es", "namespace-uri":"http://marklogic.com/entity-services"}], "range-path-index":[{"collation":"http://marklogic.com/collation/codepoint", "invalid-values":"reject", "path-expression":"//*:instance/Employee/name", "range-value-positions":false, "scalar-type":"string"}], "range-element-index":[{"collation":"http://marklogic.com/collation/en", "invalid-values":"reject", "localname":"decimal", "namespace-uri":null, "range-value-positions":false, "scalar-type":"string"}]}
Copy link
Contributor

Choose a reason for hiding this comment

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

does this need to be all on one line?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

no. but I copied it from generated code and it came that way. wasn't worth my effort to format it.

@@ -484,7 +484,7 @@ public String getHubVersion() {
return hv.getVersion();
}
catch(Exception e) {}
return "2.0.1";
return "2.0.0"; // don't change this value
Copy link
Contributor

Choose a reason for hiding this comment

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

can you update the release notes to keep this one as-is?

@marklogic-builder
Copy link

Can one of the admins verify this patch?

1 similar comment
@marklogic-builder
Copy link

Can one of the admins verify this patch?

@paxtonhare paxtonhare merged commit a4cc376 into develop Dec 12, 2017
@paxtonhare paxtonhare deleted the feature/553_update_es branch December 12, 2017 19:34
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