noliteral: don't work for some cases #21

New issue

Open

opened 2024-02-22 07:43:59 +00:00 by dkirillov · 3 comments

dkirillov commented

2024-02-22 07:43:59 +00:00

Member

We have the following log

reqLogOrDefault(r.Context(), log).Debug("policy request", zap.String("action", op),
		zap.String("resource", res), zap.String("owner", owner))

and noliteral linter doesn't complain about it but should.

noliteral version: 0.0.5
noliteral config:

noliteral:
  enable: true
  target-methods: ["Fatal"]
  disable-packages: ["codes", "tc"]
  constants-package: "git.frostfs.info/TrueCloudLab/frostfs-s3-gw/internal/logs"

We have the following [log](https://git.frostfs.info/dkirillov/frostfs-s3-gw/src/commit/8fbad3772e4c94650d7481e2b066b0238734e3c3/api/middleware/policy.go#L95-L96) ```golang reqLogOrDefault(r.Context(), log).Debug("policy request", zap.String("action", op), zap.String("resource", res), zap.String("owner", owner)) ``` and `noliteral` linter doesn't complain about it but should. noliteral version: [0.0.5](https://git.frostfs.info/TrueCloudLab/linters/releases/tag/v0.0.5) noliteral config: ```yaml noliteral: enable: true target-methods: ["Fatal"] disable-packages: ["codes", "tc"] constants-package: "git.frostfs.info/TrueCloudLab/frostfs-s3-gw/internal/logs" ```

dkirillov added the

bug

label 2024-02-22 07:43:59 +00:00

achuprov was assigned by dkirillov

2024-02-22 07:43:59 +00:00

achuprov commented

2024-03-05 08:58:36 +00:00

Member

How often do you use such a construct?
Fixing this issue might lead to a significant number of false positives. Let's make sure it's really necessary.

How often do you use such a construct? Fixing this issue might lead to a significant number of false positives. Let's make sure it's really necessary.

fyrchik commented

2024-03-05 11:21:20 +00:00

Owner

What kind of false positives are you talking about?

dkirillov commented

2024-03-06 06:41:53 +00:00

Author

Member

How often do you use such a construct?

Quite often.

Actually, I don't understand what the different between:

reqLogOrDefault(r.Context(), log).Debug("policy request", zap.String("action", op),
		zap.String("resource", res), zap.String("owner", owner))

and for example

n.reqLogger(ctx).Debug("logs.FailedToDeleteObject", zap.Stringer("cid", p.BktInfo.CID), zap.Stringer("oid", id))

But in the first case we don't get error, but in the second do.

Fixing this issue might lead to a significant number of false positives.

Maybe we can add new config value to determine additional functions name that return logger and therefore should be checked?

> How often do you use such a construct? Quite often. Actually, I don't understand what the different between: ```golang reqLogOrDefault(r.Context(), log).Debug("policy request", zap.String("action", op), zap.String("resource", res), zap.String("owner", owner)) ``` and for example ```golang n.reqLogger(ctx).Debug("logs.FailedToDeleteObject", zap.Stringer("cid", p.BktInfo.CID), zap.Stringer("oid", id)) ``` But in the first case we don't get error, but in the second do. > Fixing this issue might lead to a significant number of false positives. Maybe we can add new config value to determine additional functions name that return logger and therefore should be checked?