Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Adiciona propriedade propMaxLength ao httpConf #57

Merged
merged 5 commits into from
Aug 19, 2019

Conversation

itsdaiego
Copy link
Contributor

@itsdaiego itsdaiego commented Aug 12, 2019

Description

Add maxLengthPropproperty that will handle the max character length for a given attribute.

Currently there are two main use cases:

1 - When the body attribute exceeds it's limit:

  body: {
      foo: 'bar',
      bar: 'foo'
  },

it will be replaced by:

body: { }

2 - When the url attribute exceeds it's limit:

/myurl?prop1=value1&prop2=value2

it will be replaced by:

/myurl?prop1*** (just an example, since it may vary depending on the url's limit)

@itsdaiego itsdaiego force-pushed the add-prop-length-limit-option branch 3 times, most recently from 2d6836c to f1f3f88 Compare August 13, 2019 20:34
@itsdaiego itsdaiego changed the title WIP Adiciona propriedade propMaxLength ao httpConf Aug 13, 2019
@coveralls
Copy link

coveralls commented Aug 14, 2019

Coverage Status

Coverage decreased (-9.3%) to 77.54% when pulling 7a51bb7 on add-prop-length-limit-option into 4872c87 on master.

@mccraveiro
Copy link
Contributor

@itsdaiego consegue adicionar uns testes unitários para cobrir as linhas restantes desse arquivo?
https://coveralls.io/builds/25117694/source?filename=src/response-logger.js

src/utils.js Outdated Show resolved Hide resolved
test/unit/response-logger.js Outdated Show resolved Hide resolved
test/unit/request-logger.js Outdated Show resolved Hide resolved
Copy link
Contributor

@evaporei evaporei left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Boa 👏 💯

@itsdaiego
Copy link
Contributor Author

@itsdaiego consegue adicionar uns testes unitários para cobrir as linhas restantes desse arquivo?
https://coveralls.io/builds/25117694/source?filename=src/response-logger.js

Eu dei uma olhada, e se eu entendi bem as linhas que não estão sendo testadas, não é algo fácil de fazer agora, eu tentei testar o módulo de response inteiro e não consegui (precisaria mudar bastante o código e demoraria beem mais pra lançar essa feature) =\ mas concordo que diminuir o coverage seja algo a ser evitado, só que realmente acho que o esforço de deixar tudo 100% não é algo "trivial" (ou estou superestimando a dificuldade)

src/utils.js Outdated Show resolved Hide resolved
@itsdaiego itsdaiego force-pushed the add-prop-length-limit-option branch 2 times, most recently from 709d25a to 0ddd3d5 Compare August 14, 2019 21:32
Copy link
Contributor

@matheusvellone matheusvellone left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Na descrição o PR vc colocou que se a URL passar do tamanho, a queryString será ignorada, mas no código, vc está dando um substring na URL e concatenando *.
Senti falta dessa informação no README

src/request-logger.js Outdated Show resolved Hide resolved
src/response-logger.js Outdated Show resolved Hide resolved
src/utils.js Outdated
const filterLargeProp = (object, prop, propLengthLimit) => {
if (!object[prop]) return

return JSON.stringify(object[prop]).length > propLengthLimit ? {} : object[prop]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

E se object[prop] for um array mto grande, não deveríamos retornar [] ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Não sei se entendi, object[prop] retorna um objeto, eu converto para uma string e vejo se o length excede ao limite, o único array que existe é o propsToLog mas isso fica no src/[request/response]-logger.js

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Se você diz de colocar um limite para o tamanho da string, o que você sugere 🙃 ?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nesse ternário aqui JSON.stringify(object[prop]).length > propLengthLimit ? {} : object[prop], vc retorna sempre um objeto vazio ({}) se passar o tamanho limite. Se for um array, não deveria ser [] caso passe o tamanho ?

@itsdaiego
Copy link
Contributor Author

Na descrição o PR vc colocou que se a URL passar do tamanho, a queryString será ignorada, mas no código, vc está dando um substring na URL e concatenando *.
Senti falta dessa informação no README

Boa, foi uma alteração sugerida pelo MC em um comentário acima... ai acabei esquecendo, vou alterar, valeu! =)

src/request-logger.js Outdated Show resolved Hide resolved
src/response-logger.js Outdated Show resolved Hide resolved
const env = pickProperties(process.env, propsToLog)

const resProps = pickProperties(res, propsToLog)
resProps.body = filterLargeProp(resProps, 'body', propMaxLength.body)
reqProps.url = filterLargeUrl(reqProps.url, propMaxLength.url)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
reqProps.url = filterLargeUrl(reqProps.url, propMaxLength.url)
if (propMaxLength.url) {
reqProps.url = filterLargeUrl(reqProps.url, propMaxLength.url)
}

@itsdaiego itsdaiego force-pushed the add-prop-length-limit-option branch 3 times, most recently from b7cec20 to 7a51bb7 Compare August 19, 2019 19:45
@itsdaiego itsdaiego merged commit 6acb4e9 into master Aug 19, 2019
@mccraveiro mccraveiro deleted the add-prop-length-limit-option branch August 20, 2019 13:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants