Uploaded image for project: 'camunda BPM'
  1. camunda BPM
  2. CAM-13362

Some Freemarker variables might have a larger scope than intended

XMLWordPrintable

      Description:

      We use a few variables in our Freemarker files to configure the openApi generation. However, we also use a lot of import statements. As imports are basically just copy-pasting text in Freemarker, this can result in variables being visible in places that they probably should not be. 

      Steps to reproduce:

      Usually, this is not an issue because we reuse most variables, so old values are overwritten. On example of this becoming a problem, however, can be found here: https://github.com/camunda/camunda-bpm-platform/pull/1199/files#diff-fa91a97020a257cbd7b28dbdc612ace0c3278078bdf008f30d5fc53b1682ceb1

      sortParamsDto has to be unset in line 654, as it is visible in the global scope, due to TaskQueryDto being imported in main.ftl. If sortParamsDto is not unset, it results in SortTaskQueryParametersDto falsely being part of VariableInstanceQueryDto.

      Observed Behavior:

      If you remove line 654, and diff the new openapi.json against the old you will be able to observe the change. 

      It is possible that there are other places where a similar effect happens. This particular issue with the sortParamsDto only became apparent because the VariableInstanceQueryDto.ftl also includes the sort-props.ftl which uses this variable and because it is included in the main.ftl after the TaskQueryDto.ftl. (Due to alphabetical ordering)

      Root Cause:

      includes are generally a bad practice in Freemarker. Variables that are defined in a file that is included will be visible in the file that includes them. 

      https://freemarker.apache.org/docs/ref_directive_include.html

      https://freemarker.apache.org/docs/ref_directive_include.html#topic.import_vs_include

      Solution Ideas (Optional):

      A: Leave as is and react whenever a problem becomes apparent.

      B: Make sure that all variables that are set are unset after they are used.

      C: For all files that use or define variables, replace include with import.

      Both B and especially C are a large amount of work. However, I would suggest settling on one of the two for future endpoints.

        This is the controller panel for Smart Panels app

              anton.weltzien Anton von Weltzien
              anton.weltzien Anton von Weltzien
              Yana Vasileva Yana Vasileva
              Votes:
              0 Vote for this issue
              Watchers:
              2 Start watching this issue

                Created:
                Updated:
                Resolved: