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

Implement containsLast in HttpFields #10340

Merged
merged 44 commits into from
Aug 26, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
44 commits
Select commit Hold shift + click to select a range
a932e24
Freeze HttpFields
gregw Aug 17, 2023
04c8ab1
Merge branch 'jetty-12.0.x' into fix/10310/freezeHttpFields
gregw Aug 18, 2023
fb918c2
Better testing of iterator
gregw Aug 18, 2023
e92de8f
Implement containsLast in HttpFields
gregw Aug 18, 2023
6fc4951
Added listIterator
gregw Aug 18, 2023
d9d6dd9
Only expand request if gzip is last
gregw Aug 18, 2023
ff73edf
extra test
gregw Aug 20, 2023
03459de
Merge branch 'jetty-12.0.x' into fix/10310/freezeHttpFields
gregw Aug 20, 2023
46b11cc
Reimplemented to use a marked field rather than freeze/thaw
gregw Aug 20, 2023
88cc66c
Reimplemented to use a marked field rather than freeze/thaw
gregw Aug 20, 2023
9e936ee
Merge branch 'fix/10310/freezeHttpFields' into experiment/jetty-12.0.…
gregw Aug 20, 2023
64b2722
Persistent fields are not immutable
gregw Aug 21, 2023
c5cd147
Remove all support in HttpGenerator for Server field
gregw Aug 21, 2023
e409f44
Persistent fields are not immutable
gregw Aug 21, 2023
6f6b60b
Remove all support in HttpGenerator for Server field
gregw Aug 21, 2023
999b2b9
Merge branch 'fix/10310/freezeHttpFields' into experiment/jetty-12.0.…
gregw Aug 21, 2023
d7e8c7a
Remove all support in HttpGenerator for Server field
gregw Aug 21, 2023
7d6d400
Fix clear again
gregw Aug 21, 2023
050f351
Merge branch 'fix/10310/freezeHttpFields' into experiment/jetty-12.0.…
gregw Aug 21, 2023
3057ca5
fixed compile
gregw Aug 21, 2023
7accc62
Merge branch 'fix/10310/freezeHttpFields' into experiment/jetty-12.0.…
gregw Aug 21, 2023
ddaf2ae
Merge branch 'jetty-12.0.x' into fix/10310/freezeHttpFields
gregw Aug 21, 2023
2a83e03
Merge branch 'fix/10310/freezeHttpFields' into experiment/jetty-12.0.…
gregw Aug 21, 2023
2b71af4
Merge branch 'jetty-12.0.x' into fix/10310/freezeHttpFields
gregw Aug 21, 2023
42d297f
Merge branch 'fix/10310/freezeHttpFields' into experiment/jetty-12.0.…
gregw Aug 21, 2023
9f89b98
Merge branch 'fix/10310/freezeHttpFields' into experiment/jetty-12.0.…
gregw Aug 21, 2023
f8047cf
Don't default clear
gregw Aug 21, 2023
c2ca524
Merge branch 'fix/10310/freezeHttpFields' into experiment/jetty-12.0.…
gregw Aug 21, 2023
30d6f60
Merge branch 'jetty-12.0.x' into fix/10310/freezeHttpFields
gregw Aug 21, 2023
366cc07
Merge branch 'fix/10310/freezeHttpFields' into experiment/jetty-12.0.…
gregw Aug 21, 2023
7bd6c7d
Updates from review
gregw Aug 22, 2023
9a8dd7b
Merge remote-tracking branch 'origin/jetty-12.0.x' into experiment/je…
gregw Aug 23, 2023
2b67179
Updates from review
gregw Aug 23, 2023
c2f6717
Merge branch 'jetty-12.0.x' into fix/10310/freezeHttpFields
gregw Aug 23, 2023
484c2c0
Merge branch 'fix/10310/freezeHttpFields' into experiment/jetty-12.0.…
gregw Aug 23, 2023
2287796
Updates from review
gregw Aug 23, 2023
96b8f37
Merge branch 'jetty-12.0.x' into fix/10310/freezeHttpFields
gregw Aug 24, 2023
09db3bb
Merge branch 'fix/10310/freezeHttpFields' into experiment/jetty-12.0.…
gregw Aug 24, 2023
7348bf9
Updates from review
gregw Aug 24, 2023
e5d8859
Merge branch 'jetty-12.0.x' into fix/10310/freezeHttpFields
gregw Aug 24, 2023
61e03bb
Merge branch 'fix/10310/freezeHttpFields' into experiment/jetty-12.0.…
gregw Aug 24, 2023
2409241
Merge branch 'jetty-12.0.x' into experiment/jetty-12.0.x/HttpFieldsCo…
gregw Aug 25, 2023
66c8303
Fix bad merge
gregw Aug 25, 2023
ced7ad9
Updates from review
gregw Aug 25, 2023
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -51,19 +51,20 @@ public void beforeDecoding(Response response)
HttpResponse httpResponse = (HttpResponse)response;
httpResponse.headers(headers ->
{
ListIterator<HttpField> iterator = headers.listIterator();
while (iterator.hasNext())
boolean seenContentEncoding = false;
for (ListIterator<HttpField> iterator = headers.listIterator(headers.size()); iterator.hasPrevious();)
{
HttpField field = iterator.next();
HttpField field = iterator.previous();
HttpHeader header = field.getHeader();
if (header == HttpHeader.CONTENT_LENGTH)
{
// Content-Length is not valid anymore while we are decoding.
iterator.remove();
}
else if (header == HttpHeader.CONTENT_ENCODING)
else if (header == HttpHeader.CONTENT_ENCODING && !seenContentEncoding)
{
// Content-Encoding should be removed/modified as the content will be decoded.
// Last Content-Encoding should be removed/modified as the content will be decoded.
seenContentEncoding = true;
String value = field.getValue();
int comma = value.lastIndexOf(",");
if (comma < 0)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -258,15 +258,13 @@ protected void responseHeaders(HttpExchange exchange)
{
// Content-Encoding may have multiple values in the order they
// are applied, but we only support one decoding pass, the last one.
String contentEncoding = responseHeaders.get(HttpHeader.CONTENT_ENCODING);
String contentEncoding = responseHeaders.getLast(HttpHeader.CONTENT_ENCODING);
if (contentEncoding != null)
{
int comma = contentEncoding.indexOf(",");
if (comma > 0)
{
QuotedCSV parser = new QuotedCSV(false);
parser.addValue(contentEncoding);
List<String> values = parser.getValues();
List<String> values = new QuotedCSV(false, contentEncoding).getValues();
contentEncoding = values.get(values.size() - 1);
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -362,6 +362,56 @@ else if (c != '0' && c != '.')
return param != ZERO_QUALITY.length() && match == search.length();
}

/**
* Look for a value as the last value in a possible multivalued field
* Parameters and specifically quality parameters are not considered.
* @param search Values to search for (case-insensitive)
* @return True iff the value is contained in the field value entirely or
* as the last element of a quoted comma separated list.
*/
public boolean containsLast(String search)
{
return containsLast(getValue(), search);
}

/**
* Look for the last value in a possible multivalued field
* Parameters and specifically quality parameters are not considered.
* @param value The field value to search in.
* @param search Values to search for (case-insensitive)
* @return True iff the value is contained in the field value entirely or
* as the last element of a quoted comma separated list.
*/
public static boolean containsLast(String value, String search)
gregw marked this conversation as resolved.
Show resolved Hide resolved
{
if (search == null)
return value == null;
if (search.isEmpty())
return false;
if (value == null)
return false;
if (search.equalsIgnoreCase(value))
return true;

if (value.endsWith(search))
Copy link
Contributor

Choose a reason for hiding this comment

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

Could we do this in a better way? Here we always do an endsWith, then we possibly just create a QuotedCSV and a equalsIgnoreCase anyway.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Creating a QuotedCSV is very expensive compared to a quick endsWith check. Very few actual headers will be lists... vanishingly few. We only have the QuotedCSV path for correctness

{
int i = value.length() - search.length() - 1;
while (i >= 0)
{
char c = value.charAt(i--);
if (c == ',')
return true;
if (c != ' ')
return false;
}
return true;
}

QuotedCSV csv = new QuotedCSV(false, value);
List<String> values = csv.getValues();
return !values.isEmpty() && search.equalsIgnoreCase(values.get(values.size() - 1));
}

@Override
public int hashCode()
{
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -161,6 +161,27 @@ default HttpFields get()
return this;
}

@Override
default Iterator<HttpField> iterator()
{
return listIterator();
}

/**
* @return an iterator over the {@link HttpField}s in this {@code HttpFields}.
* @see #listIterator(int)
*/
default ListIterator<HttpField> listIterator()
{
return listIterator(0);
}

/**
* @return an iterator over the {@link HttpField}s in this {@code HttpFields} starting at the given index.
* @see #listIterator()
*/
ListIterator<HttpField> listIterator(int index);

/**
* <p>Returns an immutable copy of this {@link HttpFields} instance.</p>
*
Expand Down Expand Up @@ -245,6 +266,27 @@ default boolean contains(HttpHeader header, String value)
return false;
}

/**
* Look for a value as the last value in a possible multivalued field.
* Parameters and specifically quality parameters are not considered.
* @param header The {@link HttpHeader} type to search for.
* @param value The value to search for (case-insensitive)
* @return True iff the value is contained in the field value entirely or
* as the last element of a quoted comma separated list.
* @see HttpField#containsLast(String)
*/
default boolean containsLast(HttpHeader header, String value)
{
for (ListIterator<HttpField> i = listIterator(size()); i.hasPrevious();)
{
HttpField f = i.previous();

if (f.getHeader() == header)
return f.containsLast(value);
}
return false;
}

/**
* <p>Returns whether this instance contains the given field name
* with the given value.</p>
Expand Down Expand Up @@ -340,6 +382,28 @@ default String get(HttpHeader header)
return null;
}

/**
* <p>Returns the encoded value of the last field with the given field name,
* or {@code null} if no such header is present.</p>
* <p>In case of multi-valued fields, the returned value is the encoded
* value, including commas and quotes, as returned by {@link HttpField#getValue()}.</p>
*
* @param header the field name to search for
* @return the raw value of the last field with the given field name,
* or {@code null} if no such header is present
* @see HttpField#getValue()
*/
default String getLast(HttpHeader header)
{
for (ListIterator<HttpField> i = listIterator(size()); i.hasPrevious();)
{
HttpField f = i.previous();
if (f.getHeader() == header)
return f.getValue();
}
return null;
}

/**
* <p>Returns the encoded value of the first field with the given field name,
* or {@code null} if no such field is present.</p>
Expand Down Expand Up @@ -904,11 +968,7 @@ default Mutable add(HttpHeader header, long value)
*/
default Mutable add(HttpField field)
{
ListIterator<HttpField> i = listIterator();
while (i.hasNext())
{
i.next();
}
ListIterator<HttpField> i = listIterator(size());
i.add(field);
return this;
}
Expand Down Expand Up @@ -1041,20 +1101,6 @@ default void ensureField(HttpField field)
}
}

/**
* @return an {@link Iterator} over the {@link HttpField}s of this instance
*/
@Override
default Iterator<HttpField> iterator()
{
return listIterator();
}

/**
* @return a {@link ListIterator} over the {@link HttpField}s of this instance
*/
ListIterator<HttpField> listIterator();

/**
* <p>Puts the given {@link HttpField} into this instance.</p>
* <p>If a fields with the same name is present, the given field
Expand Down Expand Up @@ -1111,7 +1157,7 @@ default Mutable put(String name, String value)
* <p>This method behaves like {@link #remove(HttpHeader)} when
* the given {@code value} is {@code null}, otherwise behaves
* like {@link #put(HttpField)}.</p>
*
*
* @param header the name of the field
* @param value the value of the field; if {@code null} the field is removed
* @return this instance
Expand Down Expand Up @@ -1590,9 +1636,9 @@ public Mutable clear()
}

@Override
public ListIterator<HttpField> listIterator()
public ListIterator<HttpField> listIterator(int index)
{
ListIterator<HttpField> i = _fields.listIterator();
ListIterator<HttpField> i = _fields.listIterator(index);
return new ListIterator<>()
{
HttpField last;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@
package org.eclipse.jetty.http;

import java.util.Arrays;
import java.util.Iterator;
import java.util.ListIterator;
import java.util.NoSuchElementException;
import java.util.Objects;
import java.util.stream.Stream;
Expand Down Expand Up @@ -134,24 +134,9 @@ public HttpField getField(int index)
}

@Override
public Iterator<HttpField> iterator()
public ListIterator<HttpField> listIterator(int index)
gregw marked this conversation as resolved.
Show resolved Hide resolved
{
return new Iterator<>()
{
int _index = 0;

@Override
public boolean hasNext()
{
return _index < _size;
}

@Override
public HttpField next()
{
return _fields[_index++];
}
};
return new Listerator(index);
sbordet marked this conversation as resolved.
Show resolved Hide resolved
}

@Override
Expand All @@ -171,4 +156,77 @@ public String toString()
{
return asString();
}

private class Listerator implements ListIterator<HttpField>
{
private int _index;
private int _last = -1;

Listerator(int index)
{
if (index < 0 || index > _size)
throw new NoSuchElementException(Integer.toString(index));
_index = index;
gregw marked this conversation as resolved.
Show resolved Hide resolved
}

@Override
public void add(HttpField field)
{
throw new UnsupportedOperationException();
}

@Override
public boolean hasNext()
{
return _index < _size;
}

@Override
public boolean hasPrevious()
{
return _index > 0;
}

@Override
public HttpField next()
{
if (_index >= _size)
Copy link
Contributor

Choose a reason for hiding this comment

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

To be consistent with previous() I'd check here for ==.
Or both get the inequality operators.

Copy link
Contributor

Choose a reason for hiding this comment

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

There was no change here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is not really symmetric. It is plausible that _size might shrink or that index is miscalculated, so index can plausibly be larger than size. But at the other end, a negative size or index is not really plausible. But can change previous to <= 0.

throw new NoSuchElementException(Integer.toString(_index));
_last = _index++;
return _fields[_last];
}

@Override
public int nextIndex()
{
return _index + 1;
}

@Override
public HttpField previous()
{
if (_index <= 0)
throw new NoSuchElementException(Integer.toString(_index - 1));
_last = --_index;
return _fields[_last];
}

@Override
public int previousIndex()
{
return _index - 1;
}

@Override
public void remove()
{
throw new UnsupportedOperationException();
}

@Override
public void set(HttpField field)
{
throw new UnsupportedOperationException();
}
}
}
Loading