V2EX = way to explore
V2EX 是一个关于分享和探索的地方
现在注册
已注册用户请  登录
cookii
V2EX  ›  Java

记录一次修复 Spring Framework Bug 的经历

  •  
  •   cookii · 2024-09-01 22:18:38 +08:00 · 4456 次点击
    这是一个创建于 367 天前的主题,其中的信息可能已经有所发展或是发生改变。

    Spring RestTemplate 拦截器修改请求体导致的诡异问题

    最近在工作中发现了 Spring 的一个"特性"(也许可以叫 Bug ?),反正我已经给 Spring 提了 PR ,等着看能不能合进去。

    问题背景

    最近在调用第三方 API 时,遇到了一个有意思的场景。整个调用流程大概是这样的:

    1. 先调用 /login 接口,发送 username 和 password ,对方服务返回一个 JWT 。
    2. 之后的每个请求接口都是标准格式,需要把 JWT 和请求参数放到一个 JSON 中,类似这样:
    {
        "token": "JWT-TOKENxxxxxx",
        "data": {
            "key1": "value1",
            "key2": "value2"
        }
    }
    
    1. 发送请求,然后拿到响应报文。

    解决方案

    为了避免在每个接口都重复封装 token ,我想到了用 org.springframework.http.client.ClientHttpRequestInterceptor 来拦截请求,统一修改请求体。

    代码大概长这样:

    this.restTemplate = new RestTemplateBuilder()
            .requestFactory(() -> new ReactorNettyClientRequestFactory())
            .interceptors((request, body, execution) -> {
                byte[] newBody = addToken(body); // 调用登陆获取 token ,修改入参 body ,添加 token
                return execution.execute(request, newBody);
            })
            .build();
    

    诡异的问题

    修改完成后,进入测试阶段,奇怪的事情就发生了:token 能正确获取,body 也修改成功了,但对方的接口一直报 400 ,Invalid JSON 。更奇葩的是,我把 newBody 整个复制出来,用独立的 Main 代码发送请求,居然一次就成功了。

    深入源码

    不服气的我只能往源码里找原因。从RestTemplate一路 Debug 到org.springframework.http.client.InterceptingClientHttpRequest.InterceptingRequestExecution#execute,发现了这么一段代码:

    @Override
    public ClientHttpResponse execute(HttpRequest request, byte[] body) throws IOException {
        if (this.iterator.hasNext()) { //这里是在执行 interceptor 链,我的登陆和修改 body 接口就在这里执行
            ClientHttpRequestInterceptor nextInterceptor = this.iterator.next();
            return nextInterceptor.intercept(request, body, this);
        }
        else { // 上面的 interceptor 链执行完后,下面就是真实执行发送请求逻辑
            HttpMethod method = request.getMethod();
            ClientHttpRequest delegate = requestFactory.createRequest(request.getURI(), method);
            request.getHeaders().forEach((key, value) -> delegate.getHeaders().addAll(key, value)); 
            if (body.length > 0) {
                if (delegate instanceof StreamingHttpOutputMessage streamingOutputMessage) {
                    streamingOutputMessage.setBody(new StreamingHttpOutputMessage.Body() {
                        @Override
                        public void writeTo(OutputStream outputStream) throws IOException {
                            StreamUtils.copy(body, outputStream);
                        }
    
                        @Override
                        public boolean repeatable() {
                            return true;
                        }
                    });
                }
                else {
                    StreamUtils.copy(body, delegate.getBody());
                }
            }
            return delegate.execute();
        }
    }
    

    在 Debug 到request.getHeaders().forEach这里时,我突然发现 request 里的Content-Length居然和body.length(被修改后的请求体)不一样。

    问题根源

    继续往上追溯,在org.springframework.http.client.AbstractBufferingClientHttpRequest中找到了这段代码:

    @Override
    protected ClientHttpResponse executeInternal(HttpHeaders headers) throws IOException {
        byte[] bytes = this.bufferedOutput.toByteArrayUnsafe();
        if (headers.getContentLength() < 0) {
            headers.setContentLength(bytes.length);
        }
        ClientHttpResponse result = executeInternal(headers, bytes);
        this.bufferedOutput.reset();
        return result;
    }
    

    原来Content-Length在执行拦截器之前就已经被设置了。但我们在拦截器里修改了body,导致对方接收到的 JSON 格式总是不对,因为Content-Length和实际的请求体长度不匹配。

    解决问题

    这时候为了先解决问题,就先在interceptor中重新赋值了Content-Length

    this.restTemplate = new RestTemplateBuilder()
            .requestFactory(() -> new ReactorNettyClientRequestFactory())
            .interceptors((request, body, execution) -> {
                byte[] newBody = addToken(body); // 调用登陆获取 token ,修改入参 body ,添加 token
                request.getHeaders().setContentLength(body.length); // 重新设置 Content-Length
                return execution.execute(request, newBody);
            })
            .build();
    

    测试后,问题解决了。

    反思和改进

    问题虽然解决了,但我琢磨了一下,虽然是我在拦截器中修改了 body ,但这个地方 Spring 应该还是有责任把错误的Content-Length修正的。 第一,Spring 的文档中没有明确写这里应该由谁来负责,是个灰色地带。 第二,我们用RestTemplate谁会自己设置Content-Length啊,不都是框架设置的吗,所以这里不也应该由框架来负责嘛。

    思考完,周末找了个时间给 Spring 提了个 PR ,有兴趣的同学可以到这里看看。Update Content-Length when body changed by Interceptor

    有一说一,虽然不是第一次提 PR ,但是还是感觉挺爽的,记录一下。

    写的挺乱的,技术一般,大佬轻喷。

    46 条回复    2024-09-14 14:42:23 +08:00
    chebyshevj
        1
    chebyshevj  
       2024-09-01 23:20:42 +08:00
    addToken 应该是自己写的方法吧
    如果是的话,个人觉得框架本身不去更新 Content-Length 无可厚非,你既然都在自己写的方法里改了 body ,还是自己把 length 更新更好
    想起之前 EasyExcel 里面导出的表头高度问题,在拦截器里面修改表头信息不会更新表头高度,如果你新增了一行那么这行实际上不会导出,因为有有对应的 head()方法自定义表头,这样才会识别出自定义的表头高度
    因此如果 addToken 是框架本身提供的方法,那我觉得这会是一个 bug ,反之我持中立态度
    cookii
        2
    cookii  
    OP
       2024-09-01 23:28:57 +08:00 via Android
    @chebyshevj addToken 是自己的方法,但默认的 content-length 也是框架在拦截器之前设置的。这个确实不能算是 bug ,所以我也有说算个灰色地带,框架上确实可以帮你修复这个问题。
    oneisall8955
        3
    oneisall8955  
    PRO
       2024-09-01 23:31:20 +08:00   ❤️ 2
    并没有觉得是 bug 。发送前,content length 是正确的,自己改了,理应该自己维护
    StevenRCE0
        4
    StevenRCE0  
       2024-09-01 23:59:10 +08:00
    我个人支持框架将 Content-Length 封装在 interceptor 之下,也正因此不应该像 OP 这样“打补丁”实现。总之 proposal 是好的。
    chihiro2014
        5
    chihiro2014  
       2024-09-02 00:10:28 +08:00
    一般消费了请求,得复制一份才行,而且 token 一般不是放在 header 里面吗?
    deplives
        6
    deplives  
       2024-09-02 00:12:49 +08:00 via iPhone
    先抛开问题不说 jwt 一般不是在 header 里面吗 放在 body 里面的还真是头一次见
    v2exgo
        7
    v2exgo  
       2024-09-02 06:18:18 +08:00
    @StevenRCE0

    那是个 byte[] 对象,content-length 也是 byte[]的长度,大部分程序猿的框架跟语言设计,都是相信程序猿,而不是帮你处处擦屁股

    而且 op 这个 更适合在 业务代码调用 rest 封装好的那一层,做一次 AOP 拦截,反射拿到请求的对象,根据对象的 annotation 在对象里面加好 jwt ,

    为了偷懒方便 序列化后 每个对象里面都加一个 jwt , 再一次序列化回去,反正多加了也没事,多一次序列化也就多耗点时,其实还有 jwt token 泄漏的风险,因为这个 client 可能被复用 去调用到不需要 jwt 的地方
    v2exgo
        8
    v2exgo  
       2024-09-02 06:20:21 +08:00   ❤️ 1
    @deplives 现实就是这样,活久见的设计多了去了,很多时候, 很多事情都是拍脑袋决定的,何况国内公司 升职加薪 业务线完成功能就好了,基本上大家都不看技术好坏,拼的都是关系 跟 办公室政治
    v2exgo
        9
    v2exgo  
       2024-09-02 06:26:15 +08:00
    @StevenRCE0 另外在哪一层做哪一层的事情,你把对象给 restTemplate 它帮你序列化好,把序列化好的内容交给 httpclient 去处理,根据配置或者默认设置 塞好各种 header 值 这是它的职责,因为它给你提供了一层抽象,你无须操心那些细节处理,

    但是你到 interceptor 这一层来,你就跟框架到了同一层,你必须明白你在为框架添加什么功能,就跟指针一样,你必须得知道你在使用指针,何时释放它,难道编程语言设计者 没想过指针这种设计 会存在各种 手枪打到自己脚的情况?
    vvhy
        10
    vvhy  
       2024-09-02 08:33:55 +08:00 via Android   ❤️ 1
    我觉得可以把计算长度和拦截的顺序调换一下?
    wm5d8b
        11
    wm5d8b  
       2024-09-02 08:42:53 +08:00 via Android
    我记得最早 interceptor 在设计上是用来修改 header 的,后来才被搞出修改 body 的用法。spring 的问题是,没有把包括 length 在内的属性封装成一个 body 对象
    r46mht
        12
    r46mht  
       2024-09-02 08:43:19 +08:00
    @v2exgo 虽然但是,把 jwt 放到 body 里有什么坏处么...这本来也不是一个技术问题吧
    iyiluo
        13
    iyiluo  
       2024-09-02 08:58:33 +08:00
    这种 PR 估计通不过,运行了这么久的功能,即使是 bug ,也变成了 feature
    sagaxu
        14
    sagaxu  
       2024-09-02 09:02:47 +08:00
    如果是传输编码是 chunked ,setContentLength(body.length)这个 body 是分块后的吗?
    dif
        15
    dif  
       2024-09-02 09:04:41 +08:00
    既然提供了修改长度的方法,那确实很难是 bug ,不过可以发个 issue 询问一下,这一块是这样设计的,还是说是他们的疏忽。
    sagaxu
        16
    sagaxu  
       2024-09-02 09:06:47 +08:00
    @r46mht 把 jwt 放到 body 里有什么坏处么?坏处太大了,网关或中间件本来只解析 header 就能拿到身份信息,放 body 里面就不得不完整解析整个请求了,而 body 有时候比 header 大几个数量级,例如上传文件。如果在 header 里面,完全可以没接收完 http 请求时,就 reset 掉。
    cookii
        17
    cookii  
    OP
       2024-09-02 09:07:33 +08:00
    @oneisall8955
    @StevenRCE0
    @v2exgo
    @wm5d8b
    @iyiluo
    各位大佬说得对,修改了 body ,同步修改 Content-Length 是使用者的职责,但 Spring 文档没写这点。

    如果 Content-Length 和 Body 不匹配,在不同的 HttpClient 下会有不同的表现:
    如果 Content-Length > Body.length ,各种服务端基本上会等客户端发送直到超时。
    如果 Body.length > Content-Length, 有些客户端会照常发送出去,至于服务端会怎么处理就要看服务端的逻辑。有些客户端会直接报错。

    我觉得如果 Spring 框架不帮你矫正 Content-Length ,那至少也应该报错 or Warning ?
    cookii
        18
    cookii  
    OP
       2024-09-02 09:11:12 +08:00
    @sagaxu 如果是 chunked ,不能设置 ContentLength 。 不过通常 HTTP 请求很少会用到 chunked 吧,chunked 通常是响应时使用。
    r46mht
        19
    r46mht  
       2024-09-02 09:15:34 +08:00
    @sagaxu 有道理
    Chinsung
        20
    Chinsung  
       2024-09-02 09:21:58 +08:00
    bug ×
    错误的使用 √
    Martens
        21
    Martens  
       2024-09-02 09:28:43 +08:00
    content-length 有可能变化,那么 content-type 有没有可能也会变?
    nekomiao
        22
    nekomiao  
       2024-09-02 09:33:04 +08:00   ❤️ 5
    #8 这就是 V2EX 。既不是国内公司维护,主要贡献者里也没国人的影子。这都要无端端拉踩下国内公司还有人点赞。👍👍👍
    cookii
        23
    cookii  
    OP
       2024-09-02 09:37:44 +08:00
    @Martens 那确实可能也会变。
    dr1q65MfKFKHnJr6
        24
    dr1q65MfKFKHnJr6  
       2024-09-02 09:48:24 +08:00
    request 构建时修改内容,谁改谁保证功能,不应该框架去保证功能。
    securityCoding
        25
    securityCoding  
       2024-09-02 10:21:33 +08:00
    以前用 spring cloud 编解码 protobuffer 格式遇到这个问题,网关侧写的 content-length 不对
    monkeyk
        26
    monkeyk  
       2024-09-02 11:04:07 +08:00
    这不应该算是 bug ,因为这二者之间不是强绑定关系。
    并且框架也是预留了修改的点
    ala2008
        27
    ala2008  
       2024-09-02 11:15:04 +08:00
    好奇还有没有别的属性也会这样,提供了 set 方法其实可以了
    bugmakerxs
        28
    bugmakerxs  
       2024-09-02 11:39:35 +08:00
    @Chinsung 如果 content-length 在走 interceptor 之前就设置了,那么 interceptor 中不应该让使用方修改 body 内容。我倾向于是框架的 bug 。
    cookii
        29
    cookii  
    OP
       2024-09-02 13:38:39 +08:00
    @monkeyk
    @bugmakerxs
    @cedoo22
    @Chinsung
    我觉得没必要过于纠结是谁的问题, 但一个框架要让用户知道,我改了 body 的长度,也要修改 content-length 。至少文档上要写。
    cookii
        30
    cookii  
    OP
       2024-09-02 13:40:17 +08:00
    @ala2008 楼上有说,Content-Type 可能也会改变。不过这个概率更小。
    Chinsung
        31
    Chinsung  
       2024-09-02 14:31:45 +08:00
    @bugmakerxs #28 你把拦截器理解成一个非字节码层面而是 api 层面实现的切面就好了,你对一个东西修改,已经计算好的内容框架是不管你的。
    这个只存在人家设计的时候巧不巧,如果他的 content-length 是在最后计算的,那你就会为了他这么智能而感到开心,但是实际上只是运气问题而已
    xuanbg
        32
    xuanbg  
       2024-09-02 15:20:47 +08:00
    这应该算滥用拦截器了吧。。。自己重写一个 execute 方法,在这个方法里面加入 token 不好么
    pocketz
        33
    pocketz  
       2024-09-02 17:02:23 +08:00   ❤️ 1
    你既然获得了完全的 request 的控制权,修改 content length 的责任就应该完全在你。
    如果只得到了 body ,才有理由相信框架能自动修改 header
    wantstark
        34
    wantstark  
       2024-09-02 17:25:10 +08:00
    为什么不是以继承的方式呢?
    Aresxue
        35
    Aresxue  
       2024-09-02 17:52:47 +08:00
    属于滥用拦截器了,这个场景写个 Adapter 就解决了,addToken 方法里面肯定还有 json 序列化和反序列化的开销,Adapter 设计合理点还能把这部分开销优化掉。
    Vegetable
        36
    Vegetable  
       2024-09-02 18:00:16 +08:00
    不是 BUG ,合不进去
    cctv1005s927
        37
    cctv1005s927  
       2024-09-02 23:05:10 +08:00
    恭喜 merge 💐
    cookii
        38
    cookii  
    OP
       2024-09-02 23:13:53 +08:00 via Android
    @cctv1005s927 谢谢😜,我刚看见
    imaginistx12
        39
    imaginistx12  
       2024-09-03 07:42:51 +08:00
    恭喜 merge 💐
    prosgtsr
        40
    prosgtsr  
       2024-09-03 08:22:04 +08:00 via iPhone
    恭喜🎉
    billbob
        41
    billbob  
       2024-09-03 10:40:57 +08:00
    WebClient 也会有吗? RestTemplate 不是已经停止维护更新了吗?这是一个问题.
    dont39
        42
    dont39  
       2024-09-03 10:41:03 +08:00
    恭喜恭喜
    cookii
        43
    cookii  
    OP
       2024-09-03 11:14:23 +08:00
    @billbob WebClient 是 Reactor 的 client ,我没去看。RestClient 内部用的和 RestTemplate 是一样的东西,同步修复的
    xiaodehenlang
        44
    xiaodehenlang  
       364 天前
    我理解这个问题涉及到一些设计边界,比如说 body 体我开放了修改内容,那么它的长度理应是可以做到自动化设置的 这个也比较容易做到。但是像 Content-Type 这种 通过程序代码自动识别 准确度和难度都非常大,所以应该由开发者自行维护。
    xiaodehenlang
        45
    xiaodehenlang  
       364 天前
    OP 的这种精神很好,最近不忙的情况下 有些开源的代码 bug 我也学习提一下 PR
    QWE321ASD
        46
    QWE321ASD  
       354 天前
    打脸的是被合并了,充分说明有些人有多僵化
    关于   ·   帮助文档   ·   自助推广系统   ·   博客   ·   API   ·   FAQ   ·   实用小工具   ·   5060 人在线   最高记录 6679   ·     Select Language
    创意工作者们的社区
    World is powered by solitude
    VERSION: 3.9.8.5 · 26ms · UTC 01:09 · PVG 09:09 · LAX 18:09 · JFK 21:09
    Developed with CodeLauncher
    ♥ Do have faith in what you're doing.