Skip to content

Commit

Permalink
[incubator-kie-drools-6007] Executable model doesn't report an error …
Browse files Browse the repository at this point in the history
…when duplicated (apache#6013)

* removing kie-ci from dependency, because it causes a test failure in KieBaseIncludeTest

* Use canonicalKieModule.getKiePackages() rather than getKieBase()

* null check for kiePackage

* move populateIncludedRuleNameMap out of packages loop

* removed unused FileManager

* performance improvement. Use getModelForKBase instead of getKiePackages

* Fit into build phases

* clean up
  • Loading branch information
tkobayas authored and rgdoliveira committed Jul 17, 2024
1 parent 1566e87 commit d887d5c
Show file tree
Hide file tree
Showing 9 changed files with 380 additions and 6 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -225,6 +225,8 @@ public KnowledgeBuilder buildKnowledgePackages( KieBaseModelImpl kBaseModel, Bui

Set<Asset> assets = new LinkedHashSet<>();

InternalKieModule kModule = getKieModuleForKBase(kBaseModel.getName());

boolean allIncludesAreValid = true;
for (String include : getTransitiveIncludes(kBaseModel)) {
if ( StringUtils.isEmpty( include )) {
Expand All @@ -240,14 +242,18 @@ public KnowledgeBuilder buildKnowledgePackages( KieBaseModelImpl kBaseModel, Bui
}
if (compileIncludedKieBases()) {
addFiles( buildFilter, assets, getKieBaseModel( include ), includeModule, useFolders );
} else {
if (kModule != includeModule) {
// includeModule is not part of the current kModule
buildContext.addIncludeModule(getKieBaseModel(include), includeModule);
}
}
}

if (!allIncludesAreValid) {
return null;
}

InternalKieModule kModule = getKieModuleForKBase(kBaseModel.getName());
addFiles( buildFilter, assets, kBaseModel, kModule, useFolders );

KnowledgeBuilder kbuilder;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,11 @@
*/
package org.drools.compiler.kie.builder.impl;

import java.util.Collections;
import java.util.Map;

import org.kie.api.builder.model.KieBaseModel;

public class BuildContext {
private final ResultsImpl messages;

Expand All @@ -36,4 +41,12 @@ public ResultsImpl getMessages() {
public boolean registerResourceToBuild(String kBaseName, String resource) {
return true;
}

public void addIncludeModule(KieBaseModel kieBaseModel, InternalKieModule includeModule) {
// no op
}

public Map<KieBaseModel, InternalKieModule> getIncludeModules() {
return Collections.emptyMap();
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,9 @@
import java.util.Set;

import org.drools.compiler.kie.builder.impl.BuildContext;
import org.drools.compiler.kie.builder.impl.InternalKieModule;
import org.drools.compiler.kie.builder.impl.ResultsImpl;
import org.kie.api.builder.model.KieBaseModel;

public class CanonicalModelBuildContext extends BuildContext {

Expand All @@ -36,6 +38,8 @@ public class CanonicalModelBuildContext extends BuildContext {
private final Collection<GeneratedClassWithPackage> allGeneratedPojos = new HashSet<>();
private final Map<String, Class<?>> allCompiledClasses = new HashMap<>();

private final Map<KieBaseModel, InternalKieModule> includeModules = new HashMap<>();

public CanonicalModelBuildContext() { }

public CanonicalModelBuildContext(ResultsImpl messages) {
Expand Down Expand Up @@ -84,4 +88,14 @@ private String resource2Package(String resource) {
int pathEndPos = resource.lastIndexOf('/');
return pathEndPos <= 0 ? "" :resource.substring(0, pathEndPos).replace('/', '.');
}

@Override
public void addIncludeModule(KieBaseModel kieBaseModel, InternalKieModule includeModule) {
includeModules.put(kieBaseModel, includeModule);
}

@Override
public Map<KieBaseModel, InternalKieModule> getIncludeModules() {
return includeModules;
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -108,7 +108,8 @@ protected void doSecondBuildStep(Collection<CompositePackageDescr> compositePack
this.getGlobalVariableContext(),
this.sourcesGenerator,
this.packageSources,
oneClassPerRule));
oneClassPerRule,
this.getBuildContext()));

for (CompilationPhase phase : phases) {
phase.process();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,10 @@

import java.util.ArrayList;
import java.util.Collection;
import java.util.HashMap;
import java.util.List;
import java.util.Map;
import java.util.Set;
import java.util.function.Function;

import org.drools.compiler.builder.PackageRegistryManager;
Expand All @@ -33,9 +36,9 @@
import org.drools.compiler.builder.impl.processors.FunctionCompilationPhase;
import org.drools.compiler.builder.impl.processors.GlobalCompilationPhase;
import org.drools.compiler.builder.impl.processors.IteratingPhase;
import org.drools.compiler.builder.impl.processors.RuleValidator;
import org.drools.compiler.builder.impl.processors.SinglePackagePhaseFactory;
import org.drools.compiler.builder.impl.processors.WindowDeclarationCompilationPhase;
import org.drools.compiler.kie.builder.impl.BuildContext;
import org.drools.compiler.lang.descr.CompositePackageDescr;
import org.drools.kiesession.rulebase.InternalKnowledgeBase;
import org.drools.model.codegen.execmodel.PackageModel;
Expand All @@ -61,6 +64,8 @@ public class ModelMainCompilationPhase<T> implements CompilationPhase {
private final PackageSourceManager<T> packageSourceManager;
private final boolean oneClassPerRule;

private final BuildContext buildContext;

public ModelMainCompilationPhase(
PackageModelManager packageModels,
PackageRegistryManager pkgRegistryManager,
Expand All @@ -69,7 +74,11 @@ public ModelMainCompilationPhase(
boolean hasMvel,
InternalKnowledgeBase kBase,
TypeDeclarationContext typeDeclarationContext,
GlobalVariableContext globalVariableContext, Function<PackageModel, T> sourceGenerator, PackageSourceManager<T> packageSourceManager, boolean oneClassPerRule) {
GlobalVariableContext globalVariableContext,
Function<PackageModel, T> sourceGenerator,
PackageSourceManager<T> packageSourceManager,
boolean oneClassPerRule,
BuildContext buildContext) {
this.packageModels = packageModels;
this.pkgRegistryManager = pkgRegistryManager;
this.packages = packages;
Expand All @@ -81,6 +90,7 @@ public ModelMainCompilationPhase(
this.sourceGenerator = sourceGenerator;
this.packageSourceManager = packageSourceManager;
this.oneClassPerRule = oneClassPerRule;
this.buildContext = buildContext;
}

@Override
Expand All @@ -94,7 +104,9 @@ public void process() {
phases.add(iteratingPhase((reg, acc) -> GlobalCompilationPhase.of(reg, acc, kBase, globalVariableContext, acc.getFilter())));
phases.add(new DeclaredTypeDeregistrationPhase(packages, pkgRegistryManager));

phases.add(iteratingPhase((reg, acc) -> new RuleValidator(reg, acc, configuration))); // validateUniqueRuleNames
Map<String, Set<String>> includedRuleNameMap = new HashMap<>();
phases.add(new PopulateIncludedRuleNameMapPhase(buildContext.getIncludeModules(), includedRuleNameMap));
phases.add(iteratingPhase((reg, acc) -> new ModelRuleValidator(reg, acc, configuration, includedRuleNameMap))); // validateUniqueRuleNames
phases.add(iteratingPhase((reg, acc) -> new ModelGeneratorPhase(reg, acc, packageModels.getPackageModel(acc, reg, acc.getName()), typeDeclarationContext))); // validateUniqueRuleNames
phases.add(iteratingPhase((reg, acc) -> new SourceCodeGenerationPhase<>(
packageModels.getPackageModel(acc, reg, acc.getName()), packageSourceManager, sourceGenerator, oneClassPerRule))); // validateUniqueRuleNames
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,64 @@
/**
* Licensed to the Apache Software Foundation (ASF) under one
* or more contributor license agreements. See the NOTICE file
* distributed with this work for additional information
* regarding copyright ownership. The ASF licenses this file
* to you under the Apache License, Version 2.0 (the
* "License"); you may not use this file except in compliance
* with the License. You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing,
* software distributed under the License is distributed on an
* "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
* KIND, either express or implied. See the License for the
* specific language governing permissions and limitations
* under the License.
*/

package org.drools.model.codegen.execmodel.processors;

import java.util.Map;
import java.util.Set;

import org.drools.compiler.builder.impl.processors.RuleValidator;
import org.drools.compiler.compiler.PackageRegistry;
import org.drools.drl.ast.descr.PackageDescr;
import org.drools.drl.ast.descr.RuleDescr;
import org.drools.drl.parser.ParserError;
import org.kie.internal.builder.KnowledgeBuilderConfiguration;

public class ModelRuleValidator extends RuleValidator {

// extra rule names which need to be checked for duplicates.
// Non-executable model doesn't need this because included kbase assets are added to the packageDescr
// Executable model requires this because included kbase assets are modeled as separated kjar, so not added to the packageDescr
private final Map<String, Set<String>> includedRuleNameMap;

public ModelRuleValidator(PackageRegistry pkgRegistry, PackageDescr packageDescr, KnowledgeBuilderConfiguration configuration, Map<String, Set<String>> includedRuleNameMap) {
super(pkgRegistry, packageDescr, configuration);
this.includedRuleNameMap = includedRuleNameMap;
}

@Override
public void process() {
super.process();

// Check with included rule names, because exec-model doesn't add assets of included kbase to the packageDescr
String packageName = packageDescr.getNamespace();
if (includedRuleNameMap.containsKey(packageName)) {
Set<String> ruleNames = includedRuleNameMap.get(packageName);
for (final RuleDescr rule : packageDescr.getRules()) {
final String name = rule.getUnitQualifiedName();
if (ruleNames.contains(name)) {
this.results.add(new ParserError(rule.getResource(),
"Duplicate rule name: " + name,
rule.getLine(),
rule.getColumn(),
packageName));
}
}
}
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,63 @@
/**
* Licensed to the Apache Software Foundation (ASF) under one
* or more contributor license agreements. See the NOTICE file
* distributed with this work for additional information
* regarding copyright ownership. The ASF licenses this file
* to you under the Apache License, Version 2.0 (the
* "License"); you may not use this file except in compliance
* with the License. You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing,
* software distributed under the License is distributed on an
* "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
* KIND, either express or implied. See the License for the
* specific language governing permissions and limitations
* under the License.
*/
package org.drools.model.codegen.execmodel.processors;

import java.util.Collection;
import java.util.Collections;
import java.util.HashSet;
import java.util.Map;
import java.util.Set;

import org.drools.compiler.builder.impl.processors.CompilationPhase;
import org.drools.compiler.kie.builder.impl.InternalKieModule;
import org.drools.compiler.kproject.models.KieBaseModelImpl;
import org.drools.model.Model;
import org.drools.modelcompiler.CanonicalKieModule;
import org.kie.api.builder.model.KieBaseModel;
import org.kie.internal.builder.KnowledgeBuilderResult;

public class PopulateIncludedRuleNameMapPhase implements CompilationPhase {

private final Map<KieBaseModel, InternalKieModule> includeModules;
private final Map<String, Set<String>> includedRuleNameMap;

public PopulateIncludedRuleNameMapPhase(Map<KieBaseModel, InternalKieModule> includeModules, Map<String, Set<String>> includedRuleNameMap) {
this.includeModules = includeModules;
this.includedRuleNameMap = includedRuleNameMap;
}

@Override
public void process() {
for (Map.Entry<KieBaseModel, InternalKieModule> entry : includeModules.entrySet()) {
KieBaseModel kieBaseModel = entry.getKey();
InternalKieModule includeModule = entry.getValue();
if ((includeModule instanceof CanonicalKieModule canonicalKieModule) && canonicalKieModule.hasModelFile()) {
Collection<Model> includeModels = canonicalKieModule.getModelForKBase((KieBaseModelImpl)kieBaseModel);
for (Model includeModel : includeModels) {
includeModel.getRules().forEach(rule -> includedRuleNameMap.computeIfAbsent(includeModel.getPackageName(), k -> new HashSet<>()).add(rule.getName()));
}
}
}
}

@Override
public Collection<? extends KnowledgeBuilderResult> getResults() {
return Collections.emptyList();
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -466,7 +466,7 @@ private Collection<String> getRuleClassNames() {
return ruleClassesNames;
}

private Collection<Model> getModelForKBase(KieBaseModelImpl kBaseModel) {
public Collection<Model> getModelForKBase(KieBaseModelImpl kBaseModel) {
Map<String, Model> modelsMap = getModels();
if (kBaseModel.getPackages().isEmpty()) {
return modelsMap.values();
Expand All @@ -484,6 +484,11 @@ private Collection<Model> getModelForKBase(KieBaseModelImpl kBaseModel) {
return models;
}

// This method indicates if the kjar was already compiled with the executable model
public boolean hasModelFile() {
return resourceFileExists(getModelFileWithGAV(internalKieModule.getReleaseId()));
}

private Collection<String> findRuleClassesNames() {
ReleaseId releaseId = internalKieModule.getReleaseId();
String modelFiles = readExistingResourceWithName(getModelFileWithGAV(releaseId));
Expand Down
Loading

0 comments on commit d887d5c

Please sign in to comment.