Skip to content
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions docs/source/api.rst
Original file line number Diff line number Diff line change
Expand Up @@ -63,6 +63,10 @@ you need is not present!
:show-inheritance:
:members:

.. autoclass:: hyperframe.frame.ExtensionFrame
:show-inheritance:
:members:

.. autodata:: hyperframe.frame.FRAMES

Exceptions
Expand Down
48 changes: 47 additions & 1 deletion hyperframe/frame.py
Original file line number Diff line number Diff line change
Expand Up @@ -88,7 +88,7 @@ def __repr__(self):
)

@staticmethod
def parse_frame_header(header):
def parse_frame_header(header, strict=False):
Copy link
Contributor

Choose a reason for hiding this comment

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

Please add a docstring comment to explain the new parameter.

"""
Takes a 9-byte frame header and returns a tuple of the appropriate
Frame object and the length that needs to be read from the socket.
Expand All @@ -110,6 +110,14 @@ def parse_frame_header(header):
stream_id = fields[4]

if type not in FRAMES:
if not strict:
Copy link
Contributor

Choose a reason for hiding this comment

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

This is purely preference, but I’d have done this as:

if strict: raise UnknownFrameError return ExtensionFrame

because we expect strict=True to be the unusual path – or at least, it seems so, given we default to False.

(I don‘t care if you change this or not, just noting my feelings.)

frame = ExtensionFrame(
type=type,
flag_byte=flags,
stream_id=stream_id,
)
return (frame, length)

raise UnknownFrameError(type, length)

frame = FRAMES[type](stream_id)
Expand Down Expand Up @@ -732,6 +740,44 @@ def parse_body(self, data):
self.body_len = len(data)


class ExtensionFrame(Frame):
"""
ExtensionFrame is used to wrap frames which are not natively interpretable
by hyperframe.

Although certain byte prefixes are ordained by specification to have
certain contextual meanings, frames with other prefixes are not prohibited,
and may be used to communicate arbitrary meaning between HTTP 2 peers.
Copy link
Contributor

Choose a reason for hiding this comment

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

Nitpick: it’s “HTTP/2”, not “HTTP 2”.

(No, I don’t know why either.)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will fix.


Thus, hyperframe, rather than raising an exception when such a frame is
encountered, wraps it in a generic frame to be properly acted upon by
upstream consumers which might have additional context on how to use it.
"""
Copy link
Member

Choose a reason for hiding this comment

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

Can you add a versionadded stanza to this docstring?


stream_association = _STREAM_ASSOC_EITHER

def __init__(self, type, flag_byte, stream_id, **kwargs):
super(ExtensionFrame, self).__init__(stream_id, **kwargs)
self.type = type
self.flag_byte = flag_byte

def assign_flag_mapping(self, flags):
"""
When initially created, an ExtensionFrame has no concept of what flags
might be relevant to it, since a frame's flags are defined in the
specification for that frame. This method allows a frame to be assigned
a flag mapping after the fact, allowing downstream consumers to modify
ExtensionFrame to fit their needs.
"""
self.defined_flags = flags
self.flags = Flags(self.defined_flags)
return self.parse_flags(self.flag_byte)
Copy link
Member

Choose a reason for hiding this comment

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

I don't think we need this method.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

See comment above.


def parse_body(self, data):
self.body = data.tobytes()
self.body_len = len(data)


_FRAME_CLASSES = [
DataFrame,
HeadersFrame,
Expand Down
42 changes: 39 additions & 3 deletions test/test_frames.py
Original file line number Diff line number Diff line change
@@ -1,4 +1,6 @@
# -*- coding: utf-8 -*-
from hyperframe.flags import Flag

Copy link
Contributor

Choose a reason for hiding this comment

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

Nitpick: any reason for the empty line?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nope. Completely stylistic.

from hyperframe.frame import (
Frame, Flags, DataFrame, PriorityFrame, RstStreamFrame, SettingsFrame,
PushPromiseFrame, PingFrame, GoAwayFrame, WindowUpdateFrame, HeadersFrame,
Expand Down Expand Up @@ -35,10 +37,12 @@ def test_base_frame_cant_parse_body(self):
with pytest.raises(NotImplementedError):
f.parse_body(data)

def test_parse_frame_header_unknown_type(self):
def test_parse_frame_header_unknown_type_strict(self):
with pytest.raises(UnknownFrameError) as excinfo:
Frame.parse_frame_header(b'\x00\x00\x59\xFF\x00\x00\x00\x00\x01')

Frame.parse_frame_header(
b'\x00\x00\x59\xFF\x00\x00\x00\x00\x01',
strict=True
)
exception = excinfo.value
assert exception.frame_type == 0xFF
assert exception.length == 0x59
Expand All @@ -47,6 +51,38 @@ def test_parse_frame_header_unknown_type(self):
"length 89 bytes"
)

def test_parse_frame_header_unknown_type(self):
f, l = Frame.parse_frame_header(
b'\x00\x00\x59\xFF\x00\x00\x00\x00\x01'
)
assert f.type == 0xFF
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we could additionally have

assert isinstance(f, ExtensionFrame)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will add.

assert l == 0x59

def test_add_flag_options_later_unknown_type(self):
f, l = Frame.parse_frame_header(
b'\x00\x00\x59\xFF\x09\x00\x00\x00\x01'
)
assert f.type == 0xFF
assert l == 0x59
new_flags = [
Flag('FANCY_FLAG', 0x01),
Flag('REAL_THING', 0x04),
Flag('HUUUUUUGE', 0x08),
]
flags = f.assign_flag_mapping(new_flags)
assert f.defined_flags == new_flags
assert flags
assert 'FANCY_FLAG' in flags
assert 'REAL_THING' not in flags
assert 'HUUUUUUGE' in flags

def test_parse_body_unknown_type(self):
f = decode_frame(
b'\x00\x00\x0C\xFF\x00\x00\x00\x00\x01hello world!'
)
assert f.body == b'hello world!'
assert f.body_len == 12

Copy link
Member

Choose a reason for hiding this comment

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

Can at least one of these tests validate that the stream id is correct?

def test_repr(self, monkeypatch):
f = Frame(stream_id=0)
monkeypatch.setattr(Frame, "serialize_body", lambda _: b"body")
Expand Down