Bug 1066969

Summary: Unimplemented methods
Product: [Retired] JBoss BRMS Platform 6 Reporter: Tomas Schlosser <tschloss>
Component: BREAssignee: Mario Fusco <mfusco>
Status: CLOSED CURRENTRELEASE QA Contact: Tomas Schlosser <tschloss>
Severity: urgent Docs Contact:
Priority: high    
Version: unspecifiedCC: agiertli, kverlaen, mbaluch, mfusco, rrajasek
Target Milestone: ER2   
Target Release: 6.0.1   
Hardware: Unspecified   
OS: Unspecified   
Whiteboard:
Fixed In Version: Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of: Environment:
Last Closed: 2014-08-06 19:58:37 UTC Type: Bug
Regression: --- Mount Type: ---
Documentation: --- CRM:
Verified Versions: Category: ---
oVirt Team: --- RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: --- Target Upstream Version:
Embargoed:
Bug Depends On: 1045590, 1058322, 1058680    
Bug Blocks:    
Attachments:
Description Flags
List of unimplemented methods none

Description Tomas Schlosser 2014-02-19 12:31:18 UTC
Created attachment 865047 [details]
List of unimplemented methods

Description of problem:
There are a number (48 in 6.0.1.ER1) of methods that throw UnsupportedOperationException with the message "method name -> TODO". This is embarrassing to see and I already had word from GSS, customer encountered it. See attachment for actual occurences. The list was retrieved using grep -rn "TODO\"" * in root of product sources.

Version-Release number of selected component (if applicable):
BRMS 6.0.1.ER1 (sync.2014.02.10)

Comment 1 Anton Giertli 2014-02-19 12:38:51 UTC
Hi,

there are severals related bugzillas (attached).

Can we please make sure that all unimplemented methods will be implemented for 6.0.1 ?

Thanks,
Anton

Comment 3 Edson Tirelli 2014-02-19 14:38:57 UTC
We will implement the ones that make sense and change the message on the others. 

Several of these methods are left overs from previous versions that don't have equivalents or don't make sense in the new version, but I understand that the message as it is leads to confusion.

Comment 4 Anton Giertli 2014-02-19 14:48:05 UTC
Hi,

I have a customer who been using this piece of code:

int count = ksession.fireAllRules(new AgendaFilter()
            {
                @Override
                public boolean accept(Activation activation)
                {
                    boolean isAccept = activation.getRule().getName().equals("DG1UncontrolledReset"); 
                    if (isAccept) {
                        return true;
                    }
                    return false;
                }
            });

and he has hit:
java.lang.UnsupportedOperationException: org.drools.impl.adapters.StatefulKnowledgeSessionAdapter.fireAllRules -> TODO
    at org.drools.impl.adapters.StatefulKnowledgeSessionAdapter.fireAllRules(StatefulKnowledgeSessionAdapter.java:65)
    at 

Is this going to be fixed or this particular method falls under the "do not have proper equivalent" category ?

Thanks,
Anton

Comment 5 Kris Verlaenen 2014-02-19 15:22:14 UTC
Fix for
knowledge-api-legacy5-adapter/src/main/java/org/drools/impl/adapters/NodeInstanceAdapter.java:25:		throw new UnsupportedOperationException("org.drools.impl.adapters.StatefulKnowledgeSessionAdapter.getChannels -> TODO");
knowledge-api-legacy5-adapter/src/main/java/org/drools/impl/adapters/NodeInstanceAdapter.java:33:		throw new UnsupportedOperationException("org.drools.impl.adapters.StatefulKnowledgeSessionAdapter.getChannels -> TODO");
knowledge-api-legacy5-adapter/src/main/java/org/drools/impl/adapters/NodeInstanceAdapter.java:37:		throw new UnsupportedOperationException("org.drools.impl.adapters.StatefulKnowledgeSessionAdapter.getChannels -> TODO");
knowledge-api-legacy5-adapter/src/main/java/org/drools/impl/adapters/ProcessAdapter.java:53:        throw new UnsupportedOperationException("org.drools.impl.adapter.ProcessAdapter.getMetaData -> TODO");

master: http://github.com/droolsjbpm/drools/commit/33885b0c7
6.0.x: http://github.com/droolsjbpm/drools/commit/b3a156f25

Comment 6 Mario Fusco 2014-02-20 18:45:10 UTC
Fixed by https://github.com/droolsjbpm/drools/commit/baebdf122

The only still not implemented method is the one executing a Command on a session. But we no longer have the old Command implementations in our legacy api so it is impossible to adapt this to the new api.

Comment 7 Tomas Schlosser 2014-02-21 07:32:47 UTC
Hi Kris, Edson,
thank you for the work done, but there are still more (you fixed 7 out of 48). A few comments:
1) Edson it is fine if you implement only what is possible, but please change the messages in the rest of the exceptions to indicate the feature is no longer supported.
2) If you can't call execute(Command), how do you use StatelessKnowledgeSession? For simple use cases you can use execute(Collection), but it's very limiting.
3) If I see right, there is org.drools.command package in legacy adapter.

Please change the message of all exceptions before setting it to modified as I would have to return it to assigned once it landed on_qa anyway.

Comment 8 Mario Fusco 2014-02-24 12:15:57 UTC
I changed the exception message for no longer supported operations with this commit https://github.com/droolsjbpm/drools/commit/14b5cd3186ae615ec83d16ff68f2e03fc95448e5

I also confirm that execute(Command) is among those operations: in the legacy APIs we have the Command interface but only to make it compile and no implementation for this interface is provided.

Comment 9 Tomas Schlosser 2014-03-04 14:51:07 UTC
I have checked the code and all the methods with TODO are removed from all legacy classes. However I found a few such methods in following classes:

org/drools/compiler/compiler/ProjectJavaCompiler
org/drools/compiler/kie/builder/impl/KieBuilderSetImpl
org/drools/core/base/extractors/ConstantValueReader

Is it ok to have it there? If it is, I'll mark the issue VERIFIED.

Comment 10 Mario Fusco 2014-03-21 08:08:37 UTC
Those TODOs are not real TODOs, but only the default messages generated by the IDE when automatically implementing those methods. They are real unsupported operations and it is correct to throw those exception in case we invoke those methods by mistake. I agree that the exception message is misleading but anyway it is something that the user will never see. I'll change those messages, but they are totally unrelated with the legacy api implementation so I'd say that this ticket could be closed.

Comment 11 Tomas Schlosser 2014-03-24 06:57:35 UTC
Thank you Mario. This bug is now verified. I checked the code and no other "TODO" methods were found.