From f8113cb862836071cc8bdd436970312435fdb05d Mon Sep 17 00:00:00 2001 From: Robert Sparks Date: Wed, 10 May 2023 11:19:34 -0500 Subject: [PATCH] fix: close open things (#5593) * fix: close open things * fix: clean up test created files * fix: remove one close too many --- ietf/doc/tests_bofreq.py | 75 ++++++++++--------- ietf/doc/tests_review.py | 2 + ietf/ipr/management/commands/process_email.py | 10 ++- ietf/meeting/tests_views.py | 71 ++++++++++-------- .../management/commands/feedback_email.py | 7 +- ietf/nomcom/test_data.py | 6 +- ietf/nomcom/tests.py | 18 +++-- ietf/person/views.py | 16 ++-- ietf/review/mailarch.py | 1 + ietf/submit/tests.py | 24 ++++-- .../management/commands/coverage_changes.py | 1 + ietf/utils/pipe.py | 34 ++++----- ietf/utils/validators.py | 2 +- 13 files changed, 156 insertions(+), 111 deletions(-) diff --git a/ietf/doc/tests_bofreq.py b/ietf/doc/tests_bofreq.py index 9925ec3d1..2fdc8c282 100644 --- a/ietf/doc/tests_bofreq.py +++ b/ietf/doc/tests_bofreq.py @@ -320,22 +320,26 @@ This test section has some text. file = NamedTemporaryFile(delete=False,mode="w+",encoding='utf-8') file.write(f'# {username}') file.close() - for postdict in [ - {'bofreq_submission':'enter','bofreq_content':f'# {username}'}, - {'bofreq_submission':'upload','bofreq_file':open(file.name,'rb')}, - ]: - docevent_count = doc.docevent_set.count() - empty_outbox() - r = self.client.post(url, postdict) - self.assertEqual(r.status_code, 302) - doc = reload_db_objects(doc) - self.assertEqual('%02d'%(int(rev)+1) ,doc.rev) - self.assertEqual(f'# {username}', doc.text()) - self.assertEqual(docevent_count+1, doc.docevent_set.count()) - self.assertEqual(1, len(outbox)) - rev = doc.rev + try: + with open(file.name, 'rb') as bofreq_fd: + for postdict in [ + {'bofreq_submission':'enter','bofreq_content':f'# {username}'}, + {'bofreq_submission':'upload','bofreq_file':bofreq_fd}, + ]: + docevent_count = doc.docevent_set.count() + empty_outbox() + r = self.client.post(url, postdict) + self.assertEqual(r.status_code, 302) + doc = reload_db_objects(doc) + self.assertEqual('%02d'%(int(rev)+1) ,doc.rev) + self.assertEqual(f'# {username}', doc.text()) + self.assertEqual(docevent_count+1, doc.docevent_set.count()) + self.assertEqual(1, len(outbox)) + rev = doc.rev + finally: + os.unlink(file.name) + self.client.logout() - os.unlink(file.name) def test_start_new_bofreq(self): url = urlreverse('ietf.doc.views_bofreq.new_bof_request') @@ -350,25 +354,28 @@ This test section has some text. file = NamedTemporaryFile(delete=False,mode="w+",encoding='utf-8') file.write('some stuff') file.close() - for postdict in [ - dict(title='title one', bofreq_submission='enter', bofreq_content='some stuff'), - dict(title='title two', bofreq_submission='upload', bofreq_file=open(file.name,'rb')), - ]: - empty_outbox() - r = self.client.post(url, postdict) - self.assertEqual(r.status_code,302) - name = f"bofreq-{xslugify(nobody.last_name())[:64]}-{postdict['title']}".replace(' ','-') - bofreq = Document.objects.filter(name=name,type_id='bofreq').first() - self.assertIsNotNone(bofreq) - self.assertIsNotNone(DocAlias.objects.filter(name=name).first()) - self.assertEqual(bofreq.title, postdict['title']) - self.assertEqual(bofreq.rev, '00') - self.assertEqual(bofreq.get_state_slug(), 'proposed') - self.assertEqual(list(bofreq_editors(bofreq)), [nobody]) - self.assertEqual(bofreq.latest_event(NewRevisionDocEvent).rev, '00') - self.assertEqual(bofreq.text_or_error(), 'some stuff') - self.assertEqual(len(outbox),1) - os.unlink(file.name) + try: + with open(file.name,'rb') as bofreq_fd: + for postdict in [ + dict(title='title one', bofreq_submission='enter', bofreq_content='some stuff'), + dict(title='title two', bofreq_submission='upload', bofreq_file=bofreq_fd), + ]: + empty_outbox() + r = self.client.post(url, postdict) + self.assertEqual(r.status_code,302) + name = f"bofreq-{xslugify(nobody.last_name())[:64]}-{postdict['title']}".replace(' ','-') + bofreq = Document.objects.filter(name=name,type_id='bofreq').first() + self.assertIsNotNone(bofreq) + self.assertIsNotNone(DocAlias.objects.filter(name=name).first()) + self.assertEqual(bofreq.title, postdict['title']) + self.assertEqual(bofreq.rev, '00') + self.assertEqual(bofreq.get_state_slug(), 'proposed') + self.assertEqual(list(bofreq_editors(bofreq)), [nobody]) + self.assertEqual(bofreq.latest_event(NewRevisionDocEvent).rev, '00') + self.assertEqual(bofreq.text_or_error(), 'some stuff') + self.assertEqual(len(outbox),1) + finally: + os.unlink(file.name) existing_bofreq = BofreqFactory(requester_lastname=nobody.last_name()) for postdict in [ dict(title='', bofreq_submission='enter', bofreq_content='some stuff'), diff --git a/ietf/doc/tests_review.py b/ietf/doc/tests_review.py index 8cb7967cf..1f84303a9 100644 --- a/ietf/doc/tests_review.py +++ b/ietf/doc/tests_review.py @@ -499,6 +499,8 @@ class ReviewTests(TestCase): tar.add(os.path.relpath(tmp.name)) + mbox.close() + return mbox_path def test_search_mail_archive(self): diff --git a/ietf/ipr/management/commands/process_email.py b/ietf/ipr/management/commands/process_email.py index d04b54b21..0b15fb065 100644 --- a/ietf/ipr/management/commands/process_email.py +++ b/ietf/ipr/management/commands/process_email.py @@ -23,8 +23,12 @@ class Command(EmailOnFailureCommand): def handle(self, *args, **options): email = options.get('email', None) - binary_input = io.open(email, 'rb') if email else sys.stdin.buffer - self.msg_bytes = binary_input.read() + if email: + binary_input = io.open(email, 'rb') + self.msg_bytes = binary_input.read() + binary_input.close() + else: + self.msg_bytes = sys.stdin.buffer.read() try: process_response_email(self.msg_bytes) except ValueError as e: @@ -44,4 +48,4 @@ class Command(EmailOnFailureCommand): 'application', 'octet-stream', # mime type filename='original-message', ) - return msg \ No newline at end of file + return msg diff --git a/ietf/meeting/tests_views.py b/ietf/meeting/tests_views.py index 20cdaf82c..8ecec0574 100644 --- a/ietf/meeting/tests_views.py +++ b/ietf/meeting/tests_views.py @@ -894,23 +894,27 @@ class MeetingTests(BaseMeetingTestCase): def test_session_draft_tarfile(self): session, filenames = self.build_session_setup() - url = urlreverse('ietf.meeting.views.session_draft_tarfile', kwargs={'num':session.meeting.number,'acronym':session.group.acronym}) - response = self.client.get(url) - self.assertEqual(response.status_code, 200) - self.assertEqual(response.get('Content-Type'), 'application/octet-stream') - for filename in filenames: - os.unlink(filename) + try: + url = urlreverse('ietf.meeting.views.session_draft_tarfile', kwargs={'num':session.meeting.number,'acronym':session.group.acronym}) + response = self.client.get(url) + self.assertEqual(response.status_code, 200) + self.assertEqual(response.get('Content-Type'), 'application/octet-stream') + finally: + for filename in filenames: + os.unlink(filename) @skipIf(skip_pdf_tests, skip_message) @skip_coverage def test_session_draft_pdf(self): session, filenames = self.build_session_setup() - url = urlreverse('ietf.meeting.views.session_draft_pdf', kwargs={'num':session.meeting.number,'acronym':session.group.acronym}) - response = self.client.get(url) - self.assertEqual(response.status_code, 200) - self.assertEqual(response.get('Content-Type'), 'application/pdf') - for filename in filenames: - os.unlink(filename) + try: + url = urlreverse('ietf.meeting.views.session_draft_pdf', kwargs={'num':session.meeting.number,'acronym':session.group.acronym}) + response = self.client.get(url) + self.assertEqual(response.status_code, 200) + self.assertEqual(response.get('Content-Type'), 'application/pdf') + finally: + for filename in filenames: + os.unlink(filename) def test_current_materials(self): url = urlreverse('ietf.meeting.views.current_materials') @@ -6411,7 +6415,9 @@ class MaterialsTests(TestCase): path = os.path.join(submission.session.meeting.get_materials_path(),'slides') filename = os.path.join(path,session.sessionpresentation_set.first().document.name+'-01.txt') self.assertTrue(os.path.exists(filename)) - contents = io.open(filename,'r').read() + fd = io.open(filename, 'r') + contents = fd.read() + fd.close() self.assertIn('third version', contents) @@ -7946,12 +7952,13 @@ class ProceedingsTests(BaseMeetingTestCase): """Upload proceedings materials document""" meeting = self._procmat_test_meeting() for mat_type in ProceedingsMaterialTypeName.objects.filter(used=True): - mat = self.upload_proceedings_material_test( - meeting, - mat_type, - {'file': self._proceedings_file(), 'external_url': ''}, - ) - self.assertEqual(mat.get_href(), f'{mat.document.name}:00') + with self._proceedings_file() as fd: + mat = self.upload_proceedings_material_test( + meeting, + mat_type, + {'file': fd, 'external_url': ''}, + ) + self.assertEqual(mat.get_href(), f'{mat.document.name}:00') def test_add_proceedings_material_doc_invalid_ext(self): """Upload proceedings materials document with disallowed extension""" @@ -8038,12 +8045,13 @@ class ProceedingsTests(BaseMeetingTestCase): kwargs=dict(num=meeting.number, material_type=pm_doc.type.slug), ) self.client.login(username='secretary', password='secretary+password') - r = self.client.post(pm_doc_url, {'file': self._proceedings_file(), 'external_url': ''}) - self.assertRedirects(r, success_url) - self.assertEqual(meeting.proceedings_materials.count(), 2) - pm_doc = meeting.proceedings_materials.get(pk=pm_doc.pk) # refresh from DB - self.assertEqual(pm_doc.document.rev, '01') - self.assertEqual(pm_doc.get_href(), f'{pm_doc.document.name}:01') + with self._proceedings_file() as fd: + r = self.client.post(pm_doc_url, {'file': fd, 'external_url': ''}) + self.assertRedirects(r, success_url) + self.assertEqual(meeting.proceedings_materials.count(), 2) + pm_doc = meeting.proceedings_materials.get(pk=pm_doc.pk) # refresh from DB + self.assertEqual(pm_doc.document.rev, '01') + self.assertEqual(pm_doc.get_href(), f'{pm_doc.document.name}:01') # Replace the uploaded document with a URL r = self.client.post(pm_doc_url, {'use_url': 'on', 'external_url': 'https://example.com/second'}) @@ -8066,12 +8074,13 @@ class ProceedingsTests(BaseMeetingTestCase): self.assertEqual(pm_url.get_href(), 'https://example.com/third') # Now replace the URL doc with an uploaded file - r = self.client.post(pm_url_url, {'file': self._proceedings_file(), 'external_url': ''}) - self.assertRedirects(r, success_url) - self.assertEqual(meeting.proceedings_materials.count(), 2) - pm_url = meeting.proceedings_materials.get(pk=pm_url.pk) # refresh from DB - self.assertEqual(pm_url.document.rev, '02') - self.assertEqual(pm_url.get_href(), f'{pm_url.document.name}:02') + with self._proceedings_file() as fd: + r = self.client.post(pm_url_url, {'file': fd, 'external_url': ''}) + self.assertRedirects(r, success_url) + self.assertEqual(meeting.proceedings_materials.count(), 2) + pm_url = meeting.proceedings_materials.get(pk=pm_url.pk) # refresh from DB + self.assertEqual(pm_url.document.rev, '02') + self.assertEqual(pm_url.get_href(), f'{pm_url.document.name}:02') def test_remove_proceedings_material(self): """Proceedings material can be removed""" diff --git a/ietf/nomcom/management/commands/feedback_email.py b/ietf/nomcom/management/commands/feedback_email.py index 32e9c9aa2..3846208d5 100644 --- a/ietf/nomcom/management/commands/feedback_email.py +++ b/ietf/nomcom/management/commands/feedback_email.py @@ -42,8 +42,11 @@ class Command(EmailOnFailureCommand): except NomCom.DoesNotExist: raise CommandError("NomCom %s does not exist or it isn't active" % year) - binary_input = io.open(email, 'rb') if email else sys.stdin.buffer - self.msg = binary_input.read() + if email: + with io.open(email, 'rb') as binary_input: + self.msg = binary_input.read() + else: + self.msg = sys.stdin.buffer.read() try: feedback = create_feedback_email(self.nomcom, self.msg) diff --git a/ietf/nomcom/test_data.py b/ietf/nomcom/test_data.py index fc4c4fded..c969022ec 100644 --- a/ietf/nomcom/test_data.py +++ b/ietf/nomcom/test_data.py @@ -94,7 +94,8 @@ def check_comments(encryped, plain, privatekey_file): decrypted_file.close() encrypted_file.close() - decrypted_comments = io.open(decrypted_file.name, 'rb').read().decode('utf-8') + with io.open(decrypted_file.name, 'rb') as fd: + decrypted_comments = fd.read().decode('utf-8') os.unlink(encrypted_file.name) os.unlink(decrypted_file.name) @@ -116,7 +117,8 @@ def nomcom_test_data(): nomcom_test_cert_file, privatekey_file = generate_cert() nomcom.public_key.storage = FileSystemStorage(location=settings.NOMCOM_PUBLIC_KEYS_DIR) - nomcom.public_key.save('cert', File(io.open(nomcom_test_cert_file.name, 'r'))) + with io.open(nomcom_test_cert_file.name, 'r') as fd: + nomcom.public_key.save('cert', File(fd)) # chair and member create_person(group, "chair", username=CHAIR_USER, email_address='%s%s'%(CHAIR_USER,EMAIL_DOMAIN)) diff --git a/ietf/nomcom/tests.py b/ietf/nomcom/tests.py index 263551f17..8e915b74d 100644 --- a/ietf/nomcom/tests.py +++ b/ietf/nomcom/tests.py @@ -715,7 +715,8 @@ class NomcomViewsTest(TestCase): # save the cert file in tmp #nomcom.public_key.storage.location = tempfile.gettempdir() - nomcom.public_key.save('cert', File(io.open(self.cert_file.name, 'r'))) + with io.open(self.cert_file.name, 'r') as fd: + nomcom.public_key.save('cert', File(fd)) response = self.client.get(nominate_url) self.assertEqual(response.status_code, 200) @@ -781,7 +782,8 @@ class NomcomViewsTest(TestCase): # save the cert file in tmp #nomcom.public_key.storage.location = tempfile.gettempdir() - nomcom.public_key.save('cert', File(io.open(self.cert_file.name, 'r'))) + with io.open(self.cert_file.name, 'r') as fd: + nomcom.public_key.save('cert', File(fd)) response = self.client.get(nominate_url) self.assertEqual(response.status_code, 200) @@ -863,7 +865,8 @@ class NomcomViewsTest(TestCase): # save the cert file in tmp #nomcom.public_key.storage.location = tempfile.gettempdir() - nomcom.public_key.save('cert', File(io.open(self.cert_file.name, 'r'))) + with io.open(self.cert_file.name, 'r') as fd: + nomcom.public_key.save('cert', File(fd)) response = self.client.get(self.add_questionnaire_url) self.assertEqual(response.status_code, 200) @@ -942,7 +945,8 @@ class NomcomViewsTest(TestCase): # save the cert file in tmp #nomcom.public_key.storage.location = tempfile.gettempdir() - nomcom.public_key.save('cert', File(io.open(self.cert_file.name, 'r'))) + with io.open(self.cert_file.name, 'r') as fd: + nomcom.public_key.save('cert', File(fd)) response = self.client.get(feedback_url) self.assertEqual(response.status_code, 200) @@ -1066,7 +1070,8 @@ class FeedbackTest(TestCase): # save the cert file in tmp #nomcom.public_key.storage.location = tempfile.gettempdir() - nomcom.public_key.save('cert', File(io.open(self.cert_file.name, 'r'))) + with io.open(self.cert_file.name, 'r') as fd: + nomcom.public_key.save('cert', File(fd)) comment_text = 'Plain text. Comments with accents äöåÄÖÅ éáíóú âêîôû ü àèìòù.' comments = nomcom.encrypt(comment_text) @@ -1089,7 +1094,8 @@ class ReminderTest(TestCase): self.nomcom = get_nomcom_by_year(NOMCOM_YEAR) self.cert_file, self.privatekey_file = get_cert_files() #self.nomcom.public_key.storage.location = tempfile.gettempdir() - self.nomcom.public_key.save('cert', File(io.open(self.cert_file.name, 'r'))) + with io.open(self.cert_file.name, 'r') as fd: + self.nomcom.public_key.save('cert', File(fd)) gen = Position.objects.get(nomcom=self.nomcom,name='GEN') rai = Position.objects.get(nomcom=self.nomcom,name='RAI') diff --git a/ietf/person/views.py b/ietf/person/views.py index 31bf43d82..4e3f7d8c3 100644 --- a/ietf/person/views.py +++ b/ietf/person/views.py @@ -100,14 +100,14 @@ def photo(request, email_or_name): if not size.isdigit(): return HttpResponse("Size must be integer", status=400) size = int(size) - img = Image.open(person.photo) - img = img.resize((size, img.height*size//img.width)) - bytes = BytesIO() - try: - img.save(bytes, format='JPEG') - return HttpResponse(bytes.getvalue(), content_type='image/jpg') - except OSError: - raise Http404 + with Image.open(person.photo) as img: + img = img.resize((size, img.height*size//img.width)) + bytes = BytesIO() + try: + img.save(bytes, format='JPEG') + return HttpResponse(bytes.getvalue(), content_type='image/jpg') + except OSError: + raise Http404 @role_required("Secretariat") diff --git a/ietf/review/mailarch.py b/ietf/review/mailarch.py index 6ef5909a1..c34a6079c 100644 --- a/ietf/review/mailarch.py +++ b/ietf/review/mailarch.py @@ -103,6 +103,7 @@ def retrieve_messages_from_mbox(mbox_fileobj): "date": msg["Date"], "utcdate": (utcdate.date().isoformat(), utcdate.time().isoformat()) if utcdate else ("", ""), }) + mbox.close() return res diff --git a/ietf/submit/tests.py b/ietf/submit/tests.py index 9456fa3f9..f16cc5ed3 100644 --- a/ietf/submit/tests.py +++ b/ietf/submit/tests.py @@ -1561,10 +1561,16 @@ class SubmitTests(BaseSubmitTestCase): self.assertEqual(Submission.objects.filter(name=name).count(), 1) self.assertTrue(os.path.exists(os.path.join(self.staging_dir, "%s-%s.txt" % (name, rev)))) - self.assertTrue(name in io.open(os.path.join(self.staging_dir, "%s-%s.txt" % (name, rev))).read()) + fd = io.open(os.path.join(self.staging_dir, "%s-%s.txt" % (name, rev))) + txt_contents = fd.read() + fd.close() + self.assertTrue(name in txt_contents) self.assertTrue(os.path.exists(os.path.join(self.staging_dir, "%s-%s.xml" % (name, rev)))) - self.assertTrue(name in io.open(os.path.join(self.staging_dir, "%s-%s.xml" % (name, rev))).read()) - self.assertTrue('' in io.open(os.path.join(self.staging_dir, "%s-%s.xml" % (name, rev))).read()) + fd = io.open(os.path.join(self.staging_dir, "%s-%s.xml" % (name, rev))) + xml_contents = fd.read() + fd.close() + self.assertTrue(name in xml_contents) + self.assertTrue('' in xml_contents) def test_expire_submissions(self): s = Submission.objects.create(name="draft-ietf-mars-foo", @@ -3402,8 +3408,10 @@ class AsyncSubmissionTests(BaseSubmitTestCase): "test_submission.txt", title="Correct Draft Title", ) - txt_path.write_text(txt.read()) - with self.assertRaisesMessage(SubmissionError, "disagrees with submission filename"): + with txt_path.open('w') as fd: + fd.write(txt.read()) + txt.close() + with self.assertRaisesMessage(SubmissionError, 'disagrees with submission filename'): process_submission_text("draft-somebody-test", "00") # rev mismatch @@ -3414,8 +3422,10 @@ class AsyncSubmissionTests(BaseSubmitTestCase): "test_submission.txt", title="Correct Draft Title", ) - txt_path.write_text(txt.read()) - with self.assertRaisesMessage(SubmissionError, "disagrees with submission revision"): + with txt_path.open('w') as fd: + fd.write(txt.read()) + txt.close() + with self.assertRaisesMessage(SubmissionError, 'disagrees with submission revision'): process_submission_text("draft-somebody-test", "00") def test_process_and_validate_submission(self): diff --git a/ietf/utils/management/commands/coverage_changes.py b/ietf/utils/management/commands/coverage_changes.py index 75866226b..4137dbc59 100644 --- a/ietf/utils/management/commands/coverage_changes.py +++ b/ietf/utils/management/commands/coverage_changes.py @@ -73,6 +73,7 @@ class Command(BaseCommand): data = json.load(file) except ValueError as e: raise CommandError("Failure to read json data from %s: %s" % (filename, e)) + file.close() version = version or data["version"] if not version in data: raise CommandError("There is no data for version %s available in %s" % (version, filename)) diff --git a/ietf/utils/pipe.py b/ietf/utils/pipe.py index 907e4af5f..b3ac1cebf 100644 --- a/ietf/utils/pipe.py +++ b/ietf/utils/pipe.py @@ -12,23 +12,23 @@ def pipe(cmd, str=None): if str and len(str) > 4096: # XXX: Hardcoded Linux 2.4, 2.6 pipe buffer size bufsize = len(str) - pipe = Popen(cmd, stdin=PIPE, stdout=PIPE, stderr=PIPE, bufsize=bufsize, shell=True) - if not str is None: - pipe.stdin.write(str) - pipe.stdin.close() + with Popen(cmd, stdin=PIPE, stdout=PIPE, stderr=PIPE, bufsize=bufsize, shell=True) as pipe: + if not str is None: + pipe.stdin.write(str) + pipe.stdin.close() - out = b"" - err = b"" - while True: - str = pipe.stdout.read() - if str: - out += str - code = pipe.poll() - if code != None: - err = pipe.stderr.read() - break - if len(out) >= MAX: - err = "Output exceeds %s bytes and has been truncated" % MAX - break + out = b"" + err = b"" + while True: + str = pipe.stdout.read() + if str: + out += str + code = pipe.poll() + if code != None: + err = pipe.stderr.read() + break + if len(out) >= MAX: + err = "Output exceeds %s bytes and has been truncated" % MAX + break return (code, out, err) diff --git a/ietf/utils/validators.py b/ietf/utils/validators.py index 7b599b743..ec7366fd1 100644 --- a/ietf/utils/validators.py +++ b/ietf/utils/validators.py @@ -73,7 +73,7 @@ def validate_file_size(file, missing_ok=False): def validate_mime_type(file, valid, missing_ok=False): try: - file.open() + file.open() # Callers expect this to remain open. Consider refactoring. except FileNotFoundError: if missing_ok: return None, None