Hi Taher,

OK, I got back to this and finally understood what happened. I was fed up by the 
"swallowed exceptions" convo and was in a bull mindset[1].

So when you rightly wrote

>I believe the @SuppressWarnings tags are used incorrectly in this commit
>and in some cases hide the root problem.

I did not notice I wrote "Fixed some unchecked casts by using UtilMisc.toMap" 
in my commit comment.
Of course this makes no sense and I thought about using UtilGenerics.checkMap and alike. I was sure I used that, which I agree is worrying :D... Age certainly
That's why I answered "Please specify which ones you see not fit", being sure I 
did right.

Then you answered

>All of them.

This is where all derailed. Because not all changes was concerned. And then, still being in bull mindset, I did not put sufficiently attention to your last answer.

>Well, if you think using UtilGenerics or removing unused variables or
>refactoring code to remove unnecessary warnings is FUD, then I have no
>comment.

Because I sincerely though I used UtilGenerics. I somehow got mislead by Eclipse. It also hides the warning when you use UtilMisc.toMap so I did not notice and copied it everywhere.
But anyway it's not an excuse and I should have noticed the issue with List, 
which you reported in you 1st answer :/

About removing unused variables, I did not because one is really used (modelService in createFlexibleReportFromMasterServiceWorkflow). And I think the other (imageHandler in BirtUtil) will maybe used later, I put a TODO

There is though one that we can surely get rid of:
  private GenericValue userLogin;
in ReportDesignGenerator which is really useless

So I have fixed it all at, r1789381

Thanks for your review, and please accept my apologies.

Jacques
[1] Note: I believe I'm totally right about  the "swallowed exceptions" convo 
and I'll try to prove it since I'm asked so.

Le 28/03/2017 à 05:47, Jacques Le Roux a écrit :
Thanks for your detailed analysis. And yes this sentence cynical. I know 
exactly why I did so in each case, contrary as what you seem to think.

Jacques


Le 27/03/2017 à 23:05, Taher Alkhateeb a écrit :
Well, if you think using UtilGenerics or removing unused variables or
refactoring code to remove unnecessary warnings is FUD, then I have no
comment.

On Mon, Mar 27, 2017 at 4:26 PM, Jacques Le Roux <
[email protected]> wrote:

This if FUD

Jacques



Le 27/03/2017 à 13:40, Taher Alkhateeb a écrit :

All of them.

On Mon, Mar 27, 2017 at 2:29 PM, Jacques Le Roux <
[email protected]> wrote:

Please specify which ones you see not fit
Thanks

Jacques



Le 27/03/2017 à 12:14, Taher Alkhateeb a écrit :

c
and in some cases hide the root problem.

On Mon, Mar 27, 2017 at 12:54 PM, <[email protected]> wrote:

Author: jleroux

Date: Mon Mar 27 09:54:18 2017
New Revision: 1788869

URL: http://svn.apache.org/viewvc?rev=1788869&view=rev
Log:
No functional changes.

Fixes some unchecked casts by using  UtilMisc.toMap
Adds some @SuppressWarnings("unchecked")
Adds few @SuppressWarnings("unused")
Cleans imports
Completes and fixes Javadoc in BirtUtil class

Modified:
ofbiz/ofbiz-plugins/trunk/birt/src/main/java/org/apache/
ofbiz/birt/flexible/BirtMasterReportServices.java
ofbiz/ofbiz-plugins/trunk/birt/src/main/java/org/apache/
ofbiz/birt/flexible/BirtServices.java
ofbiz/ofbiz-plugins/trunk/birt/src/main/java/org/apache/
ofbiz/birt/flexible/BirtUtil.java
ofbiz/ofbiz-plugins/trunk/birt/src/main/java/org/apache/
ofbiz/birt/flexible/ReportDesignGenerator.java

Modified: ofbiz/ofbiz-plugins/trunk/birt/src/main/java/org/apache/
ofbiz/birt/flexible/BirtMasterReportServices.java
URL: http://svn.apache.org/viewvc/ofbiz/ofbiz-plugins/trunk/
birt/src/main/java/org/apache/ofbiz/birt/flexible/
BirtMasterReportServices.java?rev=1788869&r1=1788868&r2=1788
869&view=diff
============================================================
==================
--- ofbiz/ofbiz-plugins/trunk/birt/src/main/java/org/apache/
ofbiz/birt/flexible/BirtMasterReportServices.java (original)
+++ ofbiz/ofbiz-plugins/trunk/birt/src/main/java/org/apache/
ofbiz/birt/flexible/BirtMasterReportServices.java Mon Mar 27 09:54:18
2017
@@ -9,6 +9,7 @@ import java.util.List;
    import java.util.Locale;
    import java.util.Map;
    import java.util.Set;
+
    import org.apache.ofbiz.base.util.UtilDateTime;
    import org.apache.ofbiz.base.util.UtilMisc;
    import org.apache.ofbiz.base.util.UtilProperties;
@@ -32,7 +33,7 @@ public class BirtMasterReportServices {
        public static final String resource = "BirtUiLabels";
        public static final String resource_error = "BirtErrorUiLabels";

-    // The following funtion are flexible service as example for
reporting
+    // The following methods are flexible service as example for
reporting
        public static Map<String, Object> workEffortPerPersonPrepareDate
(DispatchContext
dctx, Map<String, Object> context) {
            Map<String, String> dataMap = UtilMisc.toMap("lastName",
"name",
"firstName", "name", "hours", "floating-point", "fromDate",
"date-time",
"thruDate", "date-time");
            LinkedHashMap<String, String> filterMap = new
LinkedHashMap<String, String>();
@@ -57,7 +58,7 @@ public class BirtMasterReportServices {
        public static Map<String, Object> workEffortPerPerson(DispatchCo
ntext
dctx, Map<String, Object> context) {
            Delegator delegator = (Delegator) dctx.getDelegator();
            IReportContext reportContext = (IReportContext)
context.get("reportContext");
-        Map<String, Object> parameters = (Map<String, Object>)
reportContext.getParameterValue("parameters");
+        Map<String, Object> parameters = UtilMisc.<String,
Object>toMap(reportContext.getParameterValue("parameters"));
            List<GenericValue> listWorkEffortTime = null;

            if (UtilValidate.isEmpty(parameters.get("firstName")) &&
UtilValidate.isEmpty(parameters.get("lastName"))) {
@@ -146,7 +147,7 @@ public class BirtMasterReportServices {
            Delegator delegator = (Delegator) dctx.getDelegator();
            Locale locale = (Locale) context.get("locale");
            IReportContext reportContext = (IReportContext)
context.get("reportContext");
-        Map<String, Object> parameters = (Map<String, Object>)
reportContext.getParameterValue("parameters");
+        Map<String, Object> parameters = UtilMisc.<String,
Object>toMap(reportContext.getParameterValue("parameters"));

            List<GenericValue> listTurnOver = null;
            List<Map<String, Object>> listInvoiceEditable = new
ArrayList<Map<String, Object>>();
@@ -176,7 +177,7 @@ public class BirtMasterReportServices {
                    if (parameters.get("productCategoryId") instanceof
String) {
                        String productCategoryId = (String)
parameters.get("
productCategoryId");
productCategoryList.add(productCategoryId);
-                } else {
+                } else if (parameters.get("productStoreId")
instanceof
String) {
                        productCategoryList = (List<String>)
parameters.get("
productCategoryId");
                    }
                    // getting productIds in these categories
@@ -200,7 +201,7 @@ public class BirtMasterReportServices {
                    if (parameters.get("productStoreId") instanceof
String) {
                        String productStoreId = (String)
parameters.get("
productStoreId");
productStoreList.add(productStoreId);
-                } else {
+                } else if (parameters.get("productStoreId")
instanceof
List) {
                        productStoreList = (List<String>)
parameters.get("
productStoreId");
                    }
                    // getting list of invoice Ids linked to these
productStore
@@ -259,7 +260,7 @@ public class BirtMasterReportServices {

                // adding missing fields
                for (GenericValue invoice : listTurnOver) {
-                Map<String, Object> invoiceEditableTemp = (Map<String,
Object>) invoice.clone();
+                Map<String, Object> invoiceEditableTemp =
UtilMisc.<String, Object>toMap(invoice.clone());
invoiceEditableTemp.remove("GenericEntity");
                    Map<String, Object> invoiceEditable = new
HashMap<String,
Object>();
invoiceEditable.putAll(invoiceEditableTemp);

Modified: ofbiz/ofbiz-plugins/trunk/birt/src/main/java/org/apache/
ofbiz/birt/flexible/BirtServices.java
URL: http://svn.apache.org/viewvc/ofbiz/ofbiz-plugins/trunk/
birt/src/main/java/org/apache/ofbiz/birt/flexible/
BirtServices.java?rev=1788869&r1=1788868&r2=1788869&view=diff
============================================================
==================
--- ofbiz/ofbiz-plugins/trunk/birt/src/main/java/org/apache/
ofbiz/birt/flexible/BirtServices.java (original)
+++ ofbiz/ofbiz-plugins/trunk/birt/src/main/java/org/apache/
ofbiz/birt/flexible/BirtServices.java Mon Mar 27 09:54:18 2017
@@ -163,7 +163,7 @@ public class BirtServices {
            Locale locale = (Locale) context.get("locale");
            GenericValue userLogin = (GenericValue)
context.get("userLogin");
            String entityViewName = (String) reportContext.
getParameterValue("modelElementName");
-        Map<String, Object> inputFields = (Map<String, Object>)
reportContext.getParameterValue("parameters");
+        Map<String, Object> inputFields = UtilMisc.<String,
Object>toMap(reportContext.getParameterValue("parameters"));
            Map<String, Object> resultPerformFind = new HashMap<String,
Object>();
            Map<String, Object> resultToBirt = null;
            List<GenericValue> list = null;
@@ -337,18 +337,18 @@ public class BirtServices {
                if (ServiceUtil.isError(resultMapsForGeneration)) {
                    return ServiceUtil.returnError(
ServiceUtil.getErrorMessage(resultMapsForGeneration));
                }
-            Map<String, String> dataMap = (Map<String, String>)
resultMapsForGeneration.get("dataMap");
+            Map<String, String> dataMap = UtilMisc.<String,
String>toMap(
resultMapsForGeneration.get("dataMap"));
                Map<String, String> fieldDisplayLabels = null;
                if (UtilValidate.isNotEmpty(resul
tMapsForGeneration.get("fieldDisplayLabels")))
{
-                fieldDisplayLabels = (Map<String, String>)
resultMapsForGeneration.get("fieldDisplayLabels");
+                fieldDisplayLabels = UtilMisc.<String, String>toMap(
resultMapsForGeneration.get("fieldDisplayLabels"));
                }
                Map<String, String> filterMap = null;
                if (UtilValidate.isNotEmpty(resul
tMapsForGeneration.get("filterMap")))
{
-                filterMap = (Map<String, String>)
resultMapsForGeneration.get("filterMap");
+                filterMap = UtilMisc.<String, String>toMap(
resultMapsForGeneration.get("filterMap"));
                }
                Map<String, String> filterDisplayLabels = null;
                if (UtilValidate.isNotEmpty(resul
tMapsForGeneration.get("filterDisplayLabels")))
{
-                filterDisplayLabels = (Map<String, String>)
resultMapsForGeneration.get("filterDisplayLabels");
+                filterDisplayLabels = UtilMisc.<String, String>toMap(
resultMapsForGeneration.get("filterDisplayLabels"));
                }
                contentId = BirtWorker.recordReportContent(delegator,
dispatcher, context);
                // callPerformFindFromBirt is the customMethod for
Entity
workflow
@@ -410,6 +410,7 @@ public class BirtServices {
                    serviceName = customMethodName + "PrepareFields";
                }
                try {
+                @SuppressWarnings("unused")
                    ModelService modelService = dctx.getModelService(
serviceName);
                } catch (GenericServiceException e) {
                    return ServiceUtil.returnError("No service define
with
name " + serviceName); //TODO labelise
@@ -417,10 +418,10 @@ public class BirtServices {
                contentId = BirtWorker.recordReportContent(delegator,
dispatcher, context);
                String rptDesignFileName = BirtUtil.
resolveRptDesignFilePathFromContent(delegator, contentId);
                Map<String, Object> resultService =
dispatcher.runSync(serviceName,
UtilMisc.toMap("locale", locale, "userLogin", userLogin));
-            Map<String, String> dataMap = (Map<String, String>)
resultService.get("dataMap");
-            Map<String, String> filterMap = (Map<String, String>)
resultService.get("filterMap");
-            Map<String, String> fieldDisplayLabels = (Map<String,
String>) resultService.get("fieldDisplayLabels");
-            Map<String, String> filterDisplayLabels = (Map<String,
String>) resultService.get("filterDisplayLabels");
+            Map<String, String> dataMap = UtilMisc.<String,
String>toMap(resultService.get("dataMap"));
+            Map<String, String> filterMap = UtilMisc.<String,
String>toMap(resultService.get("filterMap"));
+            Map<String, String> fieldDisplayLabels = UtilMisc.<String,
String>toMap(resultService.get("fieldDisplayLabels"));
+            Map<String, String> filterDisplayLabels =
UtilMisc.<String,
String>toMap(resultService.get("filterDisplayLabels"));
                Map<String, Object> resultGeneration =
dispatcher.runSync("createFlexibleReport",
UtilMisc.toMap(
                        "locale", locale,
                        "dataMap", dataMap,
@@ -685,6 +686,7 @@ public class BirtServices {
            if (UtilValidate.isNotEmpty(designStored.getBody())) {
                SlotHandle bodyStored = designStored.getBody();

+            @SuppressWarnings("unchecked")
                Iterator<DesignElementHandle> iter =
bodyStored.iterator();
                while (iter.hasNext()) {
                    try {
@@ -718,6 +720,7 @@ public class BirtServices {

            //copy cube
            SlotHandle cubesFromUser = designFromUser.getCubes();
+        @SuppressWarnings("unchecked")
            Iterator<DesignElementHandle> iterCube =
cubesFromUser.iterator();

            while (iterCube.hasNext()) {
@@ -733,6 +736,7 @@ public class BirtServices {

            // copy body
            SlotHandle bodyFromUser = designFromUser.getBody();
+        @SuppressWarnings("unchecked")
            Iterator<DesignElementHandle> iter =
bodyFromUser.iterator();

            while (iter.hasNext()) {
@@ -748,6 +752,7 @@ public class BirtServices {

            // deleting simple master page from design stored
            try {
+            @SuppressWarnings("unchecked")
                List<DesignElementHandle> listMasterPagesStored =
designStored.getMasterPages().getContents();
                for (Object masterPage : listMasterPagesStored) {
                    if (masterPage instanceof SimpleMasterPageHandle) {
@@ -756,6 +761,7 @@ public class BirtServices {
                }

                // adding simple master page => tous ces casts et autres
instanceof... c'est laid, mais c'est tellement galère que quand je
trouve
une solution qui marche... :s
+            @SuppressWarnings("unchecked")
                List<DesignElementHandle> listMasterPages =
designFromUser.getMasterPages().getContents();
                for (DesignElementHandle masterPage : listMasterPages) {
                    if (masterPage instanceof SimpleMasterPageHandle) {
@@ -784,12 +790,14 @@ public class BirtServices {

            // getting style names from stored report
            List<String> listStyleNames = new ArrayList<String>();
+        @SuppressWarnings("unchecked")
            Iterator<DesignElementHandle> iterStored =
stylesStored.iterator();
            while (iterStored.hasNext()) {
                DesignElementHandle item = (DesignElementHandle)
iterStored.next();
                listStyleNames.add(item.getName());
            }

+        @SuppressWarnings("unchecked")
            Iterator<DesignElementHandle> iterUser =
stylesFromUser.iterator();

            // adding to styles those which are not already present

Modified: ofbiz/ofbiz-plugins/trunk/birt/src/main/java/org/apache/
ofbiz/birt/flexible/BirtUtil.java
URL: http://svn.apache.org/viewvc/ofbiz/ofbiz-plugins/trunk/
birt/src/main/java/org/apache/ofbiz/birt/flexible/BirtUtil.
java?rev=1788869&r1=1788868&r2=1788869&view=diff
============================================================
==================
--- ofbiz/ofbiz-plugins/trunk/birt/src/main/java/org/apache/
ofbiz/birt/flexible/BirtUtil.java (original)
+++ ofbiz/ofbiz-plugins/trunk/birt/src/main/java/org/apache/
ofbiz/birt/flexible/BirtUtil.java Mon Mar 27 09:54:18 2017
@@ -18,25 +18,15 @@
************************************************************
*******************/
    package org.apache.ofbiz.birt.flexible;

-import java.io.OutputStream;
-import java.io.StringWriter;
-import java.sql.SQLException;
    import java.util.List;
-import java.util.Locale;
    import java.util.Map;
-import javax.servlet.ServletContext;
-import javax.servlet.http.HttpServletRequest;
-import javax.servlet.http.HttpServletResponse;
-import javax.servlet.http.HttpSession;
+
    import org.apache.commons.collections4.MapUtils;
-import org.apache.ofbiz.base.util.Debug;
    import org.apache.ofbiz.base.util.GeneralException;
    import org.apache.ofbiz.base.util.StringUtil;
-import org.apache.ofbiz.base.util.UtilGenerics;
    import org.apache.ofbiz.base.util.UtilMisc;
    import org.apache.ofbiz.base.util.UtilProperties;
    import org.apache.ofbiz.base.util.UtilValidate;
-import org.apache.ofbiz.base.util.template.FreeMarkerWorker;
    import org.apache.ofbiz.entity.Delegator;
    import org.apache.ofbiz.entity.GenericEntityException;
    import org.apache.ofbiz.entity.GenericValue;
@@ -44,19 +34,7 @@ import org.apache.ofbiz.entity.condition
    import org.apache.ofbiz.entity.condition.EntityConditionList;
    import org.apache.ofbiz.entity.condition.EntityExpr;
    import org.apache.ofbiz.entity.util.EntityQuery;
-import org.apache.ofbiz.entity.util.EntityUtil;
-import org.apache.ofbiz.security.Security;
-import org.apache.ofbiz.service.GenericServiceException;
-import org.apache.ofbiz.service.LocalDispatcher;
-import org.eclipse.birt.report.engine.api.EXCELRenderOption;
-import org.eclipse.birt.report.engine.api.EngineException;
-import org.eclipse.birt.report.engine.api.HTMLRenderOption;
    import org.eclipse.birt.report.engine.api.HTMLServerImageHandler;
-import org.eclipse.birt.report.engine.api.IPDFRenderOption;
-import org.eclipse.birt.report.engine.api.IReportEngine;
-import org.eclipse.birt.report.engine.api.IReportRunnable;
-import org.eclipse.birt.report.engine.api.IRunAndRenderTask;
-import org.eclipse.birt.report.engine.api.PDFRenderOption;
    import org.eclipse.birt.report.engine.api.RenderOption;
    import org.eclipse.birt.report.model.api.elements.DesignChoiceConst
ants;

@@ -64,6 +42,7 @@ public final class BirtUtil {

        public final static String module = BirtUtil.class.getName();

+    @SuppressWarnings("unused")
        private final static HTMLServerImageHandler imageHandler = new
HTMLServerImageHandler();
        private final static Map<String, String>
entityFieldTypeBirtTypeMap =
MapUtils.unmodifiableMap(UtilMisc.toMap(
                "id", DesignChoiceConstants.COLUMN_DATA_TYPE_STRING,
@@ -148,8 +127,7 @@ public final class BirtUtil {
        /**
         * Return birt field type corresponding to given entity field
type
         * @param entityFieldType
-     * @return
-     * @throws GeneralException
+     * @return birt field type corresponding to given entity field
type
         */
        public static String convertFieldTypeToBirtType(String
entityFieldType) {
            if (UtilValidate.isEmpty(entityFieldType)) {
@@ -159,10 +137,9 @@ public final class BirtUtil {
        }

        /**
-     * Return birt parameter type corresponding to given entity field
type
+     * Return birt parameter type corresponding to given entity field
type
         * @param entityFieldType
-     * @return
-     * @throws GeneralException
+     * @return birt parameter type corresponding to given entity field
type
         */
        public static String convertFieldTypeToBirtParameterType(String
entityFieldType) {
            if (UtilValidate.isEmpty(entityFieldType)) {
@@ -174,8 +151,7 @@ public final class BirtUtil {
        /**
         * Return true if mime type related to a contentType is
supported
by
Birt
         * @param contentType
-     * @return
-     * @throws GeneralException
+     * @return true if mime type related to a contentType is supported
by
Birt
         */
        public static boolean isSupportedMimeType(String contentType) {
            return mimeTypeOutputFormatMap.containsKey(contentType);
@@ -184,7 +160,7 @@ public final class BirtUtil {
        /**
         * Return mime type related to a contentType supported by Birt
         * @param contentType
-     * @return
+     * @return mime type related to a contentType supported by Birt
         * @throws GeneralException
         */
        public static String getMimeTypeOutputFormat(String contentType)
throws GeneralException {
@@ -195,9 +171,8 @@ public final class BirtUtil {
        }

        /**
-     * return extension file related to a contentType supported by
Birt
         * @param contentType
-     * @return
+     * return extension file related to a contentType supported by
Birt
         * @throws GeneralException
         */
        public static String getMimeTypeFileExtension(String
contentType)
throws GeneralException {
@@ -210,7 +185,7 @@ public final class BirtUtil {
         * second from content.properties content.upload.path.prefix
         * and add birtReptDesign directory
         * default OFBIZ_HOME/runtime/uploads/birtRptDesign/
-     * @return
+     * @return template path location where rptDesign file is stored
         */
        public static String resolveTemplatePathLocation() {
            String templatePathLocation = UtilProperties.getPropertyValu
e("birt",
"rptDesign.output.path");
@@ -231,7 +206,7 @@ public final class BirtUtil {
         * With the reporting contentId element resolve the path to
rptDesign
linked
         * @param delegator
         * @param contentId
-     * @return
+     * @return path to rptDesign file
         * @throws GenericEntityException
         */
        public static String resolveRptDesignFilePathFromCo
ntent(Delegator
delegator, String contentId) throws GenericEntityException {
@@ -253,7 +228,7 @@ public final class BirtUtil {
        /**
         * remove all non unicode alphanumeric and replace space by _
         * @param reportName
-     * @return
+     * @return spaces replaced by underscore
         */
        public static String encodeReportName(String reportName) {
            if (UtilValidate.isEmpty(reportName)) return "";

Modified: ofbiz/ofbiz-plugins/trunk/birt/src/main/java/org/apache/
ofbiz/birt/flexible/ReportDesignGenerator.java
URL: http://svn.apache.org/viewvc/ofbiz/ofbiz-plugins/trunk/
birt/src/main/java/org/apache/ofbiz/birt/flexible/
ReportDesignGenerator.java?rev=1788869&r1=1788868&r2=1788869&view=diff
============================================================
==================
--- ofbiz/ofbiz-plugins/trunk/birt/src/main/java/org/apache/
ofbiz/birt/flexible/ReportDesignGenerator.java (original)
+++ ofbiz/ofbiz-plugins/trunk/birt/src/main/java/org/apache/
ofbiz/birt/flexible/ReportDesignGenerator.java Mon Mar 27 09:54:18
2017
@@ -1,10 +1,10 @@
    package org.apache.ofbiz.birt.flexible;

-import com.ibm.icu.util.ULocale;
    import java.io.IOException;
    import java.util.LinkedHashMap;
    import java.util.Locale;
    import java.util.Map;
+
    import org.apache.ofbiz.base.util.Debug;
    import org.apache.ofbiz.base.util.GeneralException;
    import org.apache.ofbiz.base.util.UtilProperties;
@@ -40,6 +40,8 @@ import org.eclipse.birt.report.model.api
    import org.eclipse.birt.report.model.api.elements.structures.
ResultSetColumn;
    import org.eclipse.birt.report.model.elements.ReportItem;

+import com.ibm.icu.util.ULocale;
+
    /**
     * Report Design Generator Object - Handles flexible report design
Generation from Master.
     */
@@ -60,10 +62,12 @@ public class ReportDesignGenerator {
        private Map<String, String> filterDisplayLabels;
        private String rptDesignName;
        private boolean generateFilters = false;
+    @SuppressWarnings("unused")
        private GenericValue userLogin;

        public static final String resource_error = "BirtErrorUiLabels";

+    @SuppressWarnings("unchecked")
        public ReportDesignGenerator(Map<String, Object> context,
DispatchContext dctx) throws GeneralException, SemanticException {
            locale = (Locale) context.get("locale");
            dataMap = (Map<String, String>) context.get("dataMap");








Reply via email to