521 notice parcing failure #586

RollingHog · 2022-10-14T02:22:46Z

#521 adding notice for failed parsing
TESTS NOT FIXED, WAITING FOR OVERALL APPROVAL/CHANGES

UI changes:

if there are conf/runners .json files script-server cannot parse, red group called `` will appear in main scripts list

Backend changes:

list_configs() now returns mixed List[ Union[ShortConfig, GetShortConfigFailedError] ] list which is filtered in GetScripts()
server.py GetScripts() endpoint - now returns JSON {'scripts': scripts, 'failed': failed} where 'failed' is "failed parsing"

Frontend changes (I had no idea what I'm doing):

created ScriptFailedGroup.vue closely resembling ScriptListGroup.vue to show list of non-interactive unparsed files

RollingHog · 2022-10-14T23:59:32Z

And I can't see exactly which error in pretty big Travis feed fails. Never used tests in Python, sry.

bugy · 2022-10-15T08:06:45Z

Hi @RollingHog, thanks for your changes. I would see it implemented a bit differently:

Instead of returning completely different type from list_configs, I think we should just return existing short_config type with a new field "parsing_failed: true". Returning 2 completely different types from a method is more difficult to read and maintain
if a script file contains text: "allows_users", then it shouldn't be listed at all (otherwise users might see scripts, which are not intended for them)
In all other places in config_service, if we are loading extended model for a config, we should check this flag as well. I.e. if a user tries to load and display such a script, we would show proper error
On UI, instead of using a separate group, I would suggest to show the script as it is, but: make the text red and add error icon to it
Admin UI should be adjusted as well, i.e. the config should be listed, but shouldn't be clickable, and shown as read

Regarding the tests, you can run them using the follwing command from src folder:
python3 -m unittest discover -s tests -p "*.py" -t .

RollingHog · 2022-10-15T22:17:05Z

Oookay.

RollingHog · 2022-10-15T22:20:30Z

Will update this during work

~~Instead of returning completely different type from list_configs, I think we should just return existing short_config type with a new field "parsing_failed: true".~~

~~if a script file contains text: "allowed_users", then it shouldn't be listed at all (otherwise users might see scripts, which are not intended for them)~~

In all other places in config_service, if we are loading extended model for a config, we should check this flag as well. I.e. if a user tries to load and display such a script, we would show proper error

~~On UI, instead of using a separate group, I would suggest to show the script as it is, but: make the text red and add error icon to it~~

~~Admin UI should be adjusted as well, i.e. the config should be listed, but shouldn't be clickable, and shown as read~~

Kinda. Not sure what is "shown as read"

RollingHog · 2022-10-17T01:43:24Z

Questions emerged:

On allowed_users: should I regexp JSON server-side and return something like "possibly restricted script" instead of its real filename for non-admin users?
~~On Exceptions: should I hide their tracebacks from log since this is considered as acceptable option now?~~ Already did, its annoying.

…led link implementation

bugy · 2022-10-17T08:10:52Z

On allowed_users: should I regexp JSON server-side and return something like "possibly restricted script" instead of its real filename for non-admin users?

Yes please, please check the file (I think regex is not needed, just check, that the file content contains "allowed_users" string)
However, do not show anything to non-admin users at all. The file might be supposed to be hidden from them

bugy · 2022-10-17T08:11:21Z

For the stacktrace, I would suggest to log it. It will cause some noise, but it's easier to investigate having traceback

RollingHog · 2022-10-18T00:05:21Z

For the stacktrace, I would suggest to log it. It will cause some noise, but it's easier to investigate having traceback

I meant only for "cannot parse" error of course. Still log?

And about files. I think a note "some file with restricted access is malformed" will be good, wont it? Or there is some reason to fully hide this info?

bugy · 2022-10-18T09:37:52Z

I meant only for "cannot parse" error of course. Still log?

yes, it can still provide some hints on what's wrong with the file.

I think a note "some file with restricted access is malformed" will be good, wont it? Or there is some reason to fully hide this info?

Not for normal users. Any restricted information should be hidden from users, to avoid any leaks. Even if it's only a file name

RollingHog · 2022-10-18T18:32:51Z

Even if it's only a file name

No no no! What I planned to do was replacing filename with "file with possibly restricted access" server-side. So users see that something is wrong (and can eg notify admin) but don't know what exactly.

yes, it can still provide some hints on what's wrong with the file.

You mean that error message about exact line where problem is? Okay.

bugy · 2022-10-18T18:48:25Z

What I planned to do was replacing filename with "file with possibly restricted access" server-side.

Ah, I see. I thought, that you wanted to display filename and show the error
May be in this case, it would be just enough to show how many files failed to load, e.g. in the script list UI it would look like this:

Script 1
Script 2
Script 3
(red) N files failed to load

RollingHog · 2022-10-22T22:22:38Z

All done. Now trying to understand testing command output to fix the tests.

src/config/config_service.py

bugy · 2022-10-24T18:36:31Z

src/config/config_service.py

@@ -191,7 +196,10 @@ def load_script(path, content):
                    return None

                return short_config
-            except:
+            except json.decoder.JSONDecodeError:


Please add logging for the error, to help admin debugging the issue.

Isn't logging already present there?.. Line 200. It provides all info including exact mistake in JSON grammar.

I believe I wrote the comment, before you added the logging :) thanks

src/config/config_service.py

bugy · 2022-10-24T18:42:30Z

src/config/config_service.py

+                if name == path:
+                  failed_short_config = create_failed_ShortConfig(path)


I believe that we should do smth like:

failed_short_config = create_failed_ShortConfig(path) if name == failed_short_config.name: ...

I.e. we shouldn't compare the path to the name directly, because the name doesn't contain .json extension and there can be some other rules in the future

web-src/src/main-app/components/scripts/ScriptListItem.vue

bugy · 2022-10-24T19:08:01Z

web-src/src/main-app/components/scripts/ScriptListItem.vue

 .scripts-list .collection-item .menu-item-state.finished > .check-icon {
  display: block;
 }

-.scripts-list .collection-item .menu-item-state.finished > .preloader-wrapper {
+.scripts-list .collection-item .menu-item-state.finished > .preloader-wrapper,
+.scripts-list .collection-item .menu-item-state.finished > .failed-icon {


I think there is no need to duplicate this display:none multiple times.
You can define it once, smth like:

.scripts-list .collection-item .menu-item-state > .failed-icon { display: none; }

And it should be enough.
I do not know why I copied it multiple times for check icon and preload wrapper.

Not sure what to do with that, sorry.

bugy · 2022-10-24T19:10:03Z

web-src/src/main-app/store/scriptConfig.js

+                if (error.code === 422) {
+                  commit('SET_ERROR', `${CANNOT_PARSE_ERROR_PREFIX} "${selectedScript}"`);
+                  return;
+                }


I would suggest to remove custom handling here. Script server can start sending 422 for other cases as well, so always showing "parsing error" is not good. We should always rely on error.reason, which is provided by the server

Okay, how should I aquire that error then?

src/model/script_config.py

web-src/src/admin/components/scripts-config/ScriptsList.vue

Co-authored-by: Iaroslav Shepilov <buggygm@gmail.com>

…ngHog/script-server into 521-notice-parcing-failure

bugy#586 made some code review fixes for failed parsing feature

bugy · 2022-11-05T14:26:15Z

src/config/config_service.py

@@ -191,7 +196,10 @@ def load_script(path, content):
                    return None

                return short_config
-            except:
+            except json.decoder.JSONDecodeError:


I believe I wrote the comment, before you added the logging :) thanks

bugy · 2022-11-05T14:26:34Z

src/config/config_service.py

+                failed_short_config = create_failed_short_config(path)
+                return failed_short_config


Suggested change

failed_short_config = create_failed_short_config(path)

return failed_short_config

return create_failed_short_config(path)

src/config/config_service.py

bugy · 2022-11-05T14:42:22Z

src/config/config_service.py

+            except json.decoder.JSONDecodeError:
+                if name == path:
+                  failed_short_config = create_failed_ShortConfig(path)
+                  raise StopIteration(ConfigSearchResult(failed_short_config, path, None))


Not done. The exception won't be thrown here. What I meant is to add the following code after
configs = self._visit_script_configs(find_and_load)

if(search_result.short_config.parsing_failed == True): raise BadConfigFileException()

I.e. do it only once in _find_config, instead of duplicating it in load_config_model and load_config

bugy · 2022-11-05T14:43:31Z

src/model/script_config.py

+
+def create_failed_ShortConfig(path):
+    failed_short_config = ShortConfig()
+    failed_short_config.name = path


bugy · 2022-11-05T14:43:54Z

src/model/script_config.py

+    failed_short_config.name = path
+    with open(path) as f:
+      if '"allowed_users"' in f.read():
+          failed_short_config.name = 'file with possibly restricted access (path hash ' + hex(hash(path)) + ')'


Should we use some shorter name? 😅 Probably not longer than 20 characters

bugy · 2022-11-05T14:47:34Z

src/web/script_config_socket.py

@@ -184,6 +184,9 @@ def load_model():
        except ConfigNotAllowedException:
            self.close(code=403, reason='Access to the script is denied')
            return None
+        except BadConfigFileException:
+            self.close(code=BadConfigFileException.HTTP_CODE, reason=BadConfigFileException.VERBOSE_ERROR)


As discussed, please use exception.message here

bugy · 2022-11-05T14:49:13Z

web-src/src/admin/components/scripts-config/ScriptsList.vue

+          <router-link :to="script.path" append
+                       :key="script.name" 


Suggested change

<router-link :to="script.path" append

:key="script.name"

<router-link :to="script.path"

append

:key="script.name"

bugy · 2023-01-14T15:40:04Z

@RollingHog I'm so sorry, I totally forgot about this PR. I thought that we finished it.
I'm merging it to master now, thanks for your work!

RollingHog · 2023-01-15T12:54:10Z

Well, I hope it helped.

RollingHog added 4 commits October 12, 2022 05:43

initial

ddd7f41

frontend initial (tests not fixed)

aa82762

a lil clarification in group name

fa4672a

Merge branch 'bugy:master' into 521-notice-parcing-failure

a6422d5

Merge branch 'bugy:master' into 521-notice-parcing-failure

4007a47

RollingHog added 8 commits October 16, 2022 01:42

added parsing_failed to ShortConfig type (.py files only)

0cf9873

reverting vue changes

1e406ea

additional Vue reverts

56d0b8e

server.py parsing_failed parameter transmission fixed

62df429

bad script red highlight added

a33f79e

added cannot-parse icon

b0e23c6

changed "parsing-failed" icon

fa57813

yet again icon change

1d9c2b8

RollingHog added 4 commits October 17, 2022 06:49

(backend) added BadConfigFileException and (mostly) its catchers

7338f3e

(frontend) added processing of code 422 in main block

6d6e8f8

(frontend) added parsingFailed flag in ScriptsList.vue

47a80ce

(frontend) altered v-for in ScriptsList.vue for (maybe) cleaner disab…

a383704

…led link implementation

added "file with possibly restricted access" check

973af0a

bugy requested changes Oct 24, 2022

View reviewed changes

RollingHog and others added 13 commits October 25, 2022 00:44

naming convention create_failed_short_config

56be44d

added usage of BadConfigFileException.HTTP_CODE

3d533b0

Update config_service.py

eba68ce

removed some ScriptListItem.vue comments

1747d30

Update web-src/src/admin/components/scripts-config/ScriptsList.vue

c0c92a4

Co-authored-by: Iaroslav Shepilov <buggygm@gmail.com>

Update web-src/src/main-app/components/scripts/ScriptListItem.vue

bd7b49d

Co-authored-by: Iaroslav Shepilov <buggygm@gmail.com>

added path hash to failed_short_config to avoid name duplication

3521277

Merge branch '521-notice-parcing-failure' of https://github.com/Rolli…

c15fa76

…ngHog/script-server into 521-notice-parcing-failure

bugy#586 made some code review fixes for failed parsing feature

ee2a1fe

bugy#586 disabled corrupted config name obfuscation for admins

a3749b3

bugy#586 fixed tests

40bd7c7

bugy#586 fixed tests

6ddf55f

Merge pull request #1 from bugy/521-notice-parcing-failure_review_fixes

b45c224

bugy#586 made some code review fixes for failed parsing feature

bugy approved these changes Jan 14, 2023

View reviewed changes

Merge branch 'master' into 521-notice-parcing-failure

6e9d4be

bugy merged commit f7fb01a into bugy:master Jan 14, 2023

bugy added feature resolved labels Aug 4, 2023

bugy added this to the 1.18.0 milestone Aug 4, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

521 notice parcing failure #586

521 notice parcing failure #586

RollingHog commented Oct 14, 2022

RollingHog commented Oct 14, 2022

bugy commented Oct 15, 2022

RollingHog commented Oct 15, 2022 •

edited

RollingHog commented Oct 15, 2022 •

edited

RollingHog commented Oct 17, 2022 •

edited

bugy commented Oct 17, 2022

bugy commented Oct 17, 2022

RollingHog commented Oct 18, 2022 •

edited

bugy commented Oct 18, 2022

RollingHog commented Oct 18, 2022 •

edited

bugy commented Oct 18, 2022

RollingHog commented Oct 22, 2022 •

edited

bugy Oct 24, 2022

RollingHog Nov 2, 2022 •

edited

bugy Nov 5, 2022

bugy Oct 24, 2022

bugy Oct 24, 2022

RollingHog Nov 2, 2022 •

edited

bugy Oct 24, 2022

RollingHog Nov 2, 2022

bugy Nov 5, 2022

bugy Nov 5, 2022

bugy Nov 5, 2022

bugy Nov 5, 2022

bugy Nov 5, 2022

bugy Nov 5, 2022

bugy Nov 5, 2022

bugy commented Jan 14, 2023

RollingHog commented Jan 15, 2023

		if name == path:
		failed_short_config = create_failed_ShortConfig(path)

		failed_short_config = create_failed_short_config(path)
		return failed_short_config

	failed_short_config = create_failed_short_config(path)
	return failed_short_config
	return create_failed_short_config(path)

521 notice parcing failure #586

521 notice parcing failure #586

Conversation

RollingHog commented Oct 14, 2022

RollingHog commented Oct 14, 2022

bugy commented Oct 15, 2022

RollingHog commented Oct 15, 2022 • edited

RollingHog commented Oct 15, 2022 • edited

RollingHog commented Oct 17, 2022 • edited

bugy commented Oct 17, 2022

bugy commented Oct 17, 2022

RollingHog commented Oct 18, 2022 • edited

bugy commented Oct 18, 2022

RollingHog commented Oct 18, 2022 • edited

bugy commented Oct 18, 2022

RollingHog commented Oct 22, 2022 • edited

Choose a reason for hiding this comment

RollingHog Nov 2, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RollingHog Nov 2, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bugy commented Jan 14, 2023

RollingHog commented Jan 15, 2023

RollingHog commented Oct 15, 2022 •

edited

RollingHog commented Oct 15, 2022 •

edited

RollingHog commented Oct 17, 2022 •

edited

RollingHog commented Oct 18, 2022 •

edited

RollingHog commented Oct 18, 2022 •

edited

RollingHog commented Oct 22, 2022 •

edited

RollingHog Nov 2, 2022 •

edited

RollingHog Nov 2, 2022 •

edited