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

Inconsistent use of Location headers upon creating entries. #528

Open
terjekv opened this issue Apr 26, 2024 · 0 comments
Open

Inconsistent use of Location headers upon creating entries. #528

terjekv opened this issue Apr 26, 2024 · 0 comments
Labels
bug Something isn't working

Comments

@terjekv
Copy link
Collaborator

terjekv commented Apr 26, 2024

When creating a Host via post to the hosts endpoint, we get a Location header telling us were the new host is, which is done manually and repeatedly:

mreg/mreg/api/v1/views.py

Lines 349 to 408 in d45d257

def post(self, request, *args, **kwargs):
if "name" in request.data:
if self.queryset.filter(name=request.data["name"]).exists():
content = {'ERROR': 'name already in use'}
return Response(content, status=status.HTTP_409_CONFLICT)
if "ipaddress" in request.data and "network" in request.data:
content = {'ERROR': '\'ipaddress\' and \'network\' is mutually exclusive'}
return Response(content, status=status.HTTP_400_BAD_REQUEST)
# request.data is immutable
hostdata = request.data.copy()
if 'network' in hostdata:
try:
ipaddress.ip_network(hostdata['network'])
except ValueError as error:
content = {'ERROR': str(error)}
return Response(content, status=status.HTTP_400_BAD_REQUEST)
network = Network.objects.filter(network=hostdata['network']).first()
if not network:
content = {'ERROR': 'no such network'}
return Response(content, status=status.HTTP_404_NOT_FOUND)
ip = network.get_random_unused()
if not ip:
content = {'ERROR': 'no available IP in network'}
return Response(content, status=status.HTTP_404_NOT_FOUND)
hostdata['ipaddress'] = ip
del hostdata['network']
if 'ipaddress' in hostdata:
ipkey = hostdata['ipaddress']
del hostdata['ipaddress']
host = Host()
hostserializer = HostSerializer(host, data=hostdata)
if hostserializer.is_valid(raise_exception=True):
with transaction.atomic():
# XXX: must fix. perform_creates failes as it has no ipaddress and the
# the permissions fails for most users. Maybe a nested serializer should fix it?
# self.perform_create(hostserializer)
hostserializer.save()
self.save_log_create(hostserializer)
ipdata = {'host': host.pk, 'ipaddress': ipkey}
ip = Ipaddress()
ipserializer = IpaddressSerializer(ip, data=ipdata)
if ipserializer.is_valid(raise_exception=True):
self.perform_create(ipserializer)
location = request.path + host.name
return Response(status=status.HTTP_201_CREATED, headers={'Location': location})
else:
host = Host()
hostserializer = HostSerializer(host, data=hostdata)
if hostserializer.is_valid(raise_exception=True):
self.perform_create(hostserializer)
location = request.path + host.name
return Response(status=status.HTTP_201_CREATED, headers={'Location': location})

Now, we have a MregListCreateAPIView implements Location for create...

But for most list views, this class is not inherited. As one of many examples, see IPaddressList
which inherits from HostPermissionsListCreateAPIView which inherits from HostLogMixin and MregPermissionsListCreateAPIView... But none of these classes implement Location headers for post. So, using post on the ipaddress endpoint does not return a location header for the newly created entry...

Now, there may be a reason for this. We have models that look like this:

class Ipaddress(BaseModel):
    host = models.ForeignKey(
        Host, on_delete=models.CASCADE, db_column="host", related_name="ipaddresses"
    )
    ipaddress = models.GenericIPAddressField()
    macaddress = models.CharField(
        max_length=17, blank=True, validators=[validate_mac_address]
    )

    class Meta:
        db_table = "ipaddress"
        unique_together = (("host", "ipaddress"),)

So how would a location header look like? Sadly, the best we can do is /api/v1/ipaddresses/?host=<host>&ipaddress=<ipaddress>.

This is 1) inconsistent, 2) very annoying when trying to write automation, 3) missing a unique ID field that would resolve all this.

Addendum: There is an autogenerated id / pk field. We just don't return it in post. :(

@terjekv terjekv added the bug Something isn't working label Apr 26, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

1 participant