Skip to content

Commit

Permalink
v3.11.11: enhance SMUI validation (see /smui/issues/51)
Browse files Browse the repository at this point in the history
  • Loading branch information
@pbartusch
pbartusch committed Nov 13, 2020
1 parent 2f8315f commit b9274fb
Show file tree
Hide file tree
Showing 5 changed files with 245 additions and 37 deletions.
71 changes: 44 additions & 27 deletions app/assets/app/components/details/rule-management/rule-management.component.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -96,7 +96,7 @@ export class RuleManagementComponent implements OnChanges {
handleError(error: any) {
console.log('In SearchInputDetailComponent :: handleError');
console.log(':: error = ' + error);
this.showErrorMsg.emit('An error occurred.'); // TODO Do a more detaillied error description
this.showErrorMsg.emit('An error occurred.'); // TODO Do a more detailled error description
}

// TODO consider evaluate a more elegant solution to dispatch upDownDropdownDefinitionMappings from smm to the template
Expand Down Expand Up @@ -236,17 +236,7 @@ export class RuleManagementComponent implements OnChanges {
}

// take care of UP/DOWN mappings
if (this.featureToggleService.getSyncToggleUiConceptUpDownRulesCombined()) {
if ((this.detailSearchInput.upDownRules !== null) && (this.detailSearchInput.upDownRules.length > 0)) {
// convert to simple combined UP/DOWN dropdown definition mappings
// TODO consider a more elegant functional solution
for (let idxUpDownRules = 0; idxUpDownRules < this.detailSearchInput.upDownRules.length; idxUpDownRules++) {
this.detailSearchInput.upDownRules[idxUpDownRules]
.upDownDropdownDefinitionMapping = this.findIdxClosestUpDownDropdownDefinitionMapping(
this.detailSearchInput.upDownRules[idxUpDownRules]);
}
}
}
this.encodeValuesToUpDownMappings()

this.initDetailSearchInputHashForDirtyState = JSON.stringify(this.detailSearchInput); // TODO hash string value
})
Expand Down Expand Up @@ -418,22 +408,23 @@ export class RuleManagementComponent implements OnChanges {
this.detailSearchInput.redirectRules.splice(index, 1);
}

public saveSearchInputDetails() {
console.log('In SearchInputDetailComponent :: saveSearchInputDetails');

// TODO routine directly operating on this.detailSearchInput frontend model. Therefore it flickers. Refactor!

// take care of extracted Solr syntax
// WARNING: this must be done first (before UP/DOWN mappings) as below routine potentially removes 'suggestedSolrFieldName' attribute
if (this.featureToggleService.getSyncToggleUiConceptAllRulesWithSolrFields()) {
this.integrateSuggestedSolrFieldName(this.detailSearchInput.upDownRules);
this.integrateSuggestedSolrFieldName(this.detailSearchInput.filterRules);
private encodeValuesToUpDownMappings() {
if (this.featureToggleService.getSyncToggleUiConceptUpDownRulesCombined()) {
if ((this.detailSearchInput.upDownRules !== null) && (this.detailSearchInput.upDownRules.length > 0)) {
// convert to simple combined UP/DOWN dropdown definition mappings
// TODO consider a more elegant functional solution
for (let idxUpDownRules = 0; idxUpDownRules < this.detailSearchInput.upDownRules.length; idxUpDownRules++) {
this.detailSearchInput.upDownRules[idxUpDownRules]
.upDownDropdownDefinitionMapping = this.findIdxClosestUpDownDropdownDefinitionMapping(
this.detailSearchInput.upDownRules[idxUpDownRules]
)
}
}
}
}

this.updateSelectedTagsInModel()

// take care of UP/DOWN mappings
console.log(':: this.detailSearchInput.upDownRules = ' + this.detailSearchInput.upDownRules);
private decodeUpDownMappingsToValues() {
console.log('applyUpDownMappings :: this.detailSearchInput.upDownRules = ' + this.detailSearchInput.upDownRules);
if (this.featureToggleService.getSyncToggleUiConceptUpDownRulesCombined()) {
if ((this.detailSearchInput.upDownRules !== null) && (this.detailSearchInput.upDownRules.length > 0)) {
// convert from simple combined UP/DOWN dropdown definition mappings to detailed upDownType and bonus/malus value
Expand All @@ -449,9 +440,27 @@ export class RuleManagementComponent implements OnChanges {
].boostMalusValue,
isActive: upDownRule.isActive
}
});
})
}
}
}

public saveSearchInputDetails() {
console.log('In SearchInputDetailComponent :: saveSearchInputDetails');

// TODO routine directly operating on this.detailSearchInput frontend model. Therefore it flickers. Refactor!

// take care of extracted Solr syntax
// WARNING: this must be done first (before UP/DOWN mappings) as below routine potentially removes 'suggestedSolrFieldName' attribute
if (this.featureToggleService.getSyncToggleUiConceptAllRulesWithSolrFields()) {
this.integrateSuggestedSolrFieldName(this.detailSearchInput.upDownRules);
this.integrateSuggestedSolrFieldName(this.detailSearchInput.filterRules);
}

this.updateSelectedTagsInModel()

// take care of UP/DOWN mappings
this.decodeUpDownMappingsToValues()

// and persist against REST backend
this.ruleManagementService
Expand All @@ -460,6 +469,14 @@ export class RuleManagementComponent implements OnChanges {
.then(_ => this.showSuccessMsg.emit('Saving Details successful.'))
.catch(error => {
if (error.status === 400) {
console.log(':: ruleManagementService :: catch :: error = ' + JSON.stringify(error))
// take care of extracted Solr syntax and UP/DOWN mappings
if (this.featureToggleService.getSyncToggleUiConceptAllRulesWithSolrFields()) {
this.extractSuggestedSolrFieldName(this.detailSearchInput.upDownRules)
this.extractSuggestedSolrFieldName(this.detailSearchInput.filterRules)
}
this.encodeValuesToUpDownMappings()
// pass message to frontend
const msg = error.json().message;
this.saveError = msg.split('\n')
} else {
Expand Down
3 changes: 2 additions & 1 deletion app/controllers/ApiController.scala
View file
Original file line number Diff line number Diff line change
Expand Up @@ -117,11 +117,12 @@ class ApiController @Inject()(searchManagementRepository: SearchManagementReposi
case Some(strErrMsg: String) =>
logger.error("updateSearchInput failed on validation of searchInput with id " + searchInputId + " - validation returned the following error output: <<<" + strErrMsg + ">>>")
BadRequest(Json.toJson(ApiResult(API_RESULT_FAIL, strErrMsg, None)))
case None =>
case None => {
// TODO handle potential conflict between searchInputId and JSON-passed searchInput.id
searchManagementRepository.updateSearchInput(searchInput)
// TODO consider Update returning the updated SearchInput(...) instead of an ApiResult(...)
Ok(Json.toJson(ApiResult(API_RESULT_OK, "Updating Search Input successful.", Some(SearchInputId(searchInputId)))))
}
}
}.getOrElse {
BadRequest(Json.toJson(ApiResult(API_RESULT_FAIL, "Adding new Search Input failed. Unexpected body data.", None)))
Expand Down
66 changes: 59 additions & 7 deletions app/models/querqy/QuerqyRulesTxtGenerator.scala
View file
Original file line number Diff line number Diff line change
Expand Up @@ -202,7 +202,37 @@ class QuerqyRulesTxtGenerator @Inject()(searchManagementRepository: SearchManage
}
}


def validateNativeQuery(ruleTerm: String): Option[String] = {
// if there is a native query configured (indicated by *)
val posNativeQueryIndicator = ruleTerm.indexOf('*')
if (posNativeQueryIndicator != -1) {
// ... then is this to be non empty
val nativeQuery = ruleTerm.substring(posNativeQueryIndicator+1).trim
if (nativeQuery.isEmpty) {
// Intuitively this should be a validation error as well, but this constellation is covered by querqy itself.
// So, this validation will accept empty native queries as querqy does the validation (see validateSearchInputToErrMsg).
None
} else {
// TODO this is Solr only (and might need to be adjusted, in case of native Elasticsearch queries)
// if there is a field/value indicator present
val posFieldValueIndicator = nativeQuery.indexOf(':')
if (posFieldValueIndicator != -1) {
// ... then this needs to be followed by a (non-empty) FIELD_NAME:FIELD_VALUE pattern
val fieldName = nativeQuery.substring(0, posFieldValueIndicator).trim
val fieldValue = nativeQuery.substring(posFieldValueIndicator+1).trim
if(fieldName.isEmpty || fieldValue.isEmpty) {
Some(s"No FIELD_NAME:FIELD_VALUE pattern given for native query rule = $ruleTerm")
} else {
None
}
} else {
None
}
}
} else {
None
}
}

/**
* Validate a {{searchInput}} instance for (1) SMUI plausibility as well as (2) the resulting rules.txt fragment
Expand All @@ -211,13 +241,10 @@ class QuerqyRulesTxtGenerator @Inject()(searchManagementRepository: SearchManage
* @param searchInput Input instance to be validated.
* @return None, if no validation error, otherwise a String containing the error.
*/
// TODO maybe the validation concern should be moved into the input model?
def validateSearchInputToErrMsg(searchInput: SearchInputWithRules): Option[String] = {

// TODO validation ends with first broken rule, it should collect all errors to a line.
// TODO decide, if input having no rule at all is legit ... (e.g. newly created). Will currently being filtered.

// validate against SMUI plausibility rules
// TODO evaluate to refactor the validation implementation into models/QuerqyRulesTxtGenerator

// if input contains *-Wildcard, all synonyms must be directed
// TODO discuss if (1) contains or (2) endsWith is the right interpretation
Expand All @@ -234,11 +261,35 @@ class QuerqyRulesTxtGenerator @Inject()(searchManagementRepository: SearchManage

val redirectRuleErrors = searchInput.redirectRules.map(r => validateRedirectTarget(r.target))

val nativeQueryValidationError = (
// native search-engine queries can occur with UP/DOWN and FILTER rules
searchInput.upDownRules
++ searchInput.filterRules
).map(r => validateNativeQuery(r.term))

val emptyRuleValidationError = (searchInput.synonymRules ++ searchInput.upDownRules ++ searchInput.filterRules ++ searchInput.deleteRules)
.map {
case r: RuleWithTerm => r
}
.map(r => {
if(r.term.trim.isEmpty) {
Some(s"Rule '${r.term}' is empty")
} else {
None
}
})

// validate as well against querqy parser

val querqyRuleValidationError = validateQuerqyRulesTxtToErrMsg(renderSearchInputRulesForTerm(searchInput.term, searchInput))

val errors = List(querqyRuleValidationError, undirectedSynonymsCheck, synonymsDirectedCheck) ++ redirectRuleErrors
// aggregate validation feedback

// finally validate as well against querqy parser
val errors = List(
querqyRuleValidationError,
undirectedSynonymsCheck,
synonymsDirectedCheck
) ++ redirectRuleErrors ++ nativeQueryValidationError ++ emptyRuleValidationError

// TODO validate both inputs and rules, for all undirected synonym terms in this input
errors.foldLeft[Option[String]](None) {
Expand Down Expand Up @@ -267,4 +318,5 @@ class QuerqyRulesTxtGenerator @Inject()(searchManagementRepository: SearchManage
case Failure(t: Throwable) => Some(s"Error validating $target: ${t.getMessage}")
}
}

}
2 changes: 1 addition & 1 deletion build.sbt
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ import com.typesafe.sbt.GitBranchPrompt
import com.typesafe.sbt.packager.rpm.RpmPlugin.autoImport.{rpmBrpJavaRepackJars, rpmLicense}

name := "search-management-ui"
version := "3.11.10"
version := "3.11.11"

scalaVersion := "2.12.11"

Expand Down
140 changes: 139 additions & 1 deletion test/models/QuerqyRulesTxtGeneratorSpec.scala
View file
Original file line number Diff line number Diff line change
Expand Up @@ -219,6 +219,144 @@ class QuerqyRulesTxtGeneratorSpec extends FlatSpec with Matchers with MockitoSug
Some("Line 31: Cannot parse line: ADD AN INVALID INSTRUCTION")
}

// TODO add tests for validateSearchInputToErrMsg
/**
* Validation extension spec, see https://github.com/querqy/smui/issues/51
*/

def staticDownRuleWithTerm(term: String) = {
UpDownRule(
upDownType = UpDownRule.TYPE_DOWN,
boostMalusValue = 50,
term = term,
isActive = true
)
}

def staticFilterRuleWithTerm(term: String) = {
FilterRule(
term = term,
isActive = true
)
}

def staticSynonymRuleWithTerm(term: String) = {
SynonymRule(
synonymType = SynonymRule.TYPE_UNDIRECTED,
term = term,
isActive = true
)
}

def staticDeleteRuleWithTerm(term: String) = {
DeleteRule(
term = term,
isActive = true
)
}

"SearchInputWithRules with empty rules" should "return a validation error" in {
val invalidInput = SearchInputWithRules(
id = SearchInputId(),
term = "test query",
synonymRules = List(
// add this to make the rules.txt partial valid in general (for querqy validation)
staticSynonymRuleWithTerm("valid synonym"),
// start with the invalid rules (SMUI validated)
staticSynonymRuleWithTerm(""),
staticSynonymRuleWithTerm(" ")
),
upDownRules = List(
staticDownRuleWithTerm(""),
staticDownRuleWithTerm(" ")
),
filterRules = List(
staticFilterRuleWithTerm(""),
staticFilterRuleWithTerm(" ")
),
deleteRules = List(
staticDeleteRuleWithTerm(""),
staticDeleteRuleWithTerm(" ")
),
redirectRules = Nil,
tags = Seq.empty,
isActive = true,
comment = ""
)

val errors = generator.validateSearchInputToErrMsg(invalidInput)

errors shouldBe Some("Rule '' is empty, Rule ' ' is empty, Rule '' is empty, Rule ' ' is empty, Rule '' is empty, Rule ' ' is empty, Rule '' is empty, Rule ' ' is empty")
}

"SearchInputWithRules with invalid native queries" should "return a validation error" in {

val invalidInput = SearchInputWithRules(
id = SearchInputId(),
term = "test query",
synonymRules = Nil,
upDownRules = List(
staticDownRuleWithTerm("*"),
staticDownRuleWithTerm(" * "),
staticDownRuleWithTerm("* manufacturer:"),
staticDownRuleWithTerm("* :Wiko"),
staticDownRuleWithTerm("* manufacturer: "),
staticDownRuleWithTerm("* :Wiko"),
),
filterRules = List(
staticFilterRuleWithTerm("*"),
staticFilterRuleWithTerm(" * "),
staticFilterRuleWithTerm("* -searchText:"),
staticFilterRuleWithTerm("* :\"Mi Smart Plug\""),
staticFilterRuleWithTerm("* -searchText: "),
staticFilterRuleWithTerm("* :\"Mi Smart Plug\"")
),
deleteRules = Nil,
redirectRules = Nil,
tags = Seq.empty,
isActive = true,
comment = ""
)

val errors = generator.validateSearchInputToErrMsg(invalidInput)

// TODO There should also be a "Missing raw query" validation error for "Line 3" and 8 and 9 (first FILTER expressions), where there is also no native query - maybe a querqy issue?
errors shouldBe Some(
"Line 2: Missing raw query after * in line: DOWN(50): *, No FIELD_NAME:FIELD_VALUE pattern given for native query rule = * manufacturer:, No FIELD_NAME:FIELD_VALUE pattern given for native query rule = * :Wiko, No FIELD_NAME:FIELD_VALUE pattern given for native query rule = * manufacturer: , No FIELD_NAME:FIELD_VALUE pattern given for native query rule = * :Wiko, No FIELD_NAME:FIELD_VALUE pattern given for native query rule = * -searchText:, No FIELD_NAME:FIELD_VALUE pattern given for native query rule = * :\"Mi Smart Plug\", No FIELD_NAME:FIELD_VALUE pattern given for native query rule = * -searchText: , No FIELD_NAME:FIELD_VALUE pattern given for native query rule = * :\"Mi Smart Plug\""
)
}

"SearchInputWithRules with valid native queries" should "return no validation error" in {

val validInput = SearchInputWithRules(
id = SearchInputId(),
term = "test query",
synonymRules = Nil,
upDownRules = List(
staticDownRuleWithTerm("* a:a"),
staticDownRuleWithTerm("* a: a "),
staticDownRuleWithTerm("* manufacturer:Wiko"),
staticDownRuleWithTerm("manufacturer:ZTE"),
staticDownRuleWithTerm("searchText:\"Iphone 11\"")
),
filterRules = List(
staticFilterRuleWithTerm("* a:a"),
staticFilterRuleWithTerm("* a: a "),
staticFilterRuleWithTerm("* -searchText:\"Mi Smart Plug\"\""),
staticFilterRuleWithTerm("* -searchText:\"Roboter-Staubsauger\"")

),
deleteRules = Nil,
redirectRules = Nil,
tags = Seq.empty,
isActive = true,
comment = ""
)

val errors = generator.validateSearchInputToErrMsg(validInput)

errors shouldBe None
}

// TODO add more tests for validateSearchInputToErrMsg

}

0 comments on commit b9274fb

Please sign in to comment.