quize1小作業 #1

dirtyrabbit · 2023-03-22T15:56:11Z

No description provided.

hsinhoyeh · 2023-03-24T02:02:25Z

@dirtyrabbit thanks for pursuing this challenge, but before we move any further, please purify your PR, keep it as small as possible: Normally a PR should contain necessary changes in the code, not in the state. Also, the size (in terms of the number of files) should NOT exceed hundreds, not even to say thousands. Please remove unnecessary files. Do we need files underquiz01/data/ to be able to review this code change? And the python cache file has the same situation.

hsinhoyeh · 2023-03-24T02:03:24Z

quiz/01-rate-limit/quiz01/core/tests.py

+        time.sleep(2)
+        print('-----------Get not over-----------')
+        for i in range(1,101):
+            resp = self.c.get('/api/')


how do you guarantee these 100 API calls would be executed in one second?

In normal circumstances, the testing time will not exceed 1 second, so no special handling is required. In the new version, a judgment of execution time has been added. When the execution time exceeds 1 second, the test will be performed again. If the execution time is less than 1 second or has been tested 5 times (if it cannot be less than 1 second, an error will be displayed), the testing loop will be break or terminated.

for i in range(1,5): t0 = int(time.time()) // request // t1 = int(time.time()) if((t1-t0) <1): break

this would trigger infinite testing when the worse scenario has happened. why not just fail the test case?

Sorry, could you provide more details about the 'worse scenario'? If you mean that the execution time consistently fails to be less than 1 second, in this case, the loop will be terminated after the 5th iteration, which can be observed from 'for i in range(1,5)'.

def test_get_not_over(self): print('-----------Get not over-----------') for i in range(1,5): // reset api limit // t0 = int(time.time()) #number of request for j in range(1,101): // request api // t1 = int(time.time()) # if execution time < 1, don't do again if((t1-t0) <1): break if((t1-t0) >1): self.assertEqual(0, 1) #we can't get it for less than 1s else: self.assertEqual(resp.status_code, 200)

hsinhoyeh · 2023-03-24T02:06:40Z

quiz/01-rate-limit/quiz01/core/tests.py

+    # Unover rate limiting get test 
+    # will be get 200 status
+    def test_get_not_over(self):
+        time.sleep(2)


time.sleep(2) is used to clean up the previous state.
however, that means a test runner would take 2 more seconds to be able to complete this test.
are we able to make it act like a state machine? like

serverstub.resetRateLimitConstraint($ip or not) for i in range(1,101): resp = self.c.get('/api/')

Indeed, it would be better to switch to that behavior. However, it's unfortunate that django-ratelimit doesn't seem to provide that functionality directly. Therefore, in the new version, we have created that functionality ourselves, which can be viewed reset function in the 'core/view' . It has also been added to the tests, improving our testing process.
result:

resp = self.c.post('/reset/',{'group':'get', 'key':'ip', 'rate':'100/s', 'method':'GET'}) for j in range(1,101): resp = self.c.get('/api/') #request api

hsinhoyeh · 2023-03-24T02:07:24Z

quiz/01-rate-limit/quiz01/core/tests.py

+        print('-----------Get not over-----------')
+        for i in range(1,101):
+            resp = self.c.get('/api/')
+        self.assertEqual(resp.status_code, 200)


besides http_code, we need to verify the response header too.

No problem, we can use response["header-name"] to retrieve the header file. However, to keep the testing simple, we separate that test into its own independent one.

# response type of header file is string # resp['X-RateLimit-Limit'] = '1' resp['X-RateLimit-Remaining'] = '0' resp['X-RateLimit-Reset'] = '0' self.assertEqual(resp['X-RateLimit-Limit']+resp['X-RateLimit-Remaining']+resp['X-RateLimit-Reset'], '100') or self.assertEqual(resp['Retry-At'], '0')

dirtyrabbit · 2023-03-28T12:07:58Z

Thank you for your response and suggestion. In the new version, the gitignore has been refactored and created with reference to gitignore.io. The quiz01/data/ has been ignored because it will be created locally during execution and the changes in the database can be observed during execution. Therefore, The data and the program are separated and will not cause any impact.

hsinhoyeh · 2023-03-30T03:10:29Z

@dirtyrabbit here are some comments on the code changes. thanks
let me know you idea if you think differently.

dirtyrabbit · 2023-03-30T07:35:47Z

It has been replied. Any comments are welcome to be raised.

hsinhoyeh

I just realized my review are not submitted until now. sorry for the delay.

hsinhoyeh · 2023-03-30T02:49:44Z

LICENSE

this file should not be touched. why you are removing it?

hsinhoyeh · 2023-03-30T02:50:00Z

README.md

this file should not be touched. why you are removing it?

Sorry, I didn't include these files when testing 'gitignore' earlier. They have now been restored.

hsinhoyeh · 2023-03-30T02:52:38Z

quiz/01-rate-limit/quiz01/Dockerfile

+FROM python:3
+ENV PYTHONDONTWRITEBYTECODE=1
+ENV PYTHONUNBUFFERED=1
+WORKDIR /code


what we would do is

ADD requirements.txt /code/ RUN pip install -r requirements.txt ADD . /code/

making requirements.txt to be the top layer of the container image would leverage image cache when you are re-built it.

In the previous version, it was stated that according to the Dockerfile best practices guide, we should always prefer COPY over ADD unless we specifically need one of the two additional features of ADD. However, based on our requirements, we have changed it to ADD.

that's why we need a code review, seniority can NOT surpass the best practice.
Thanks for pointing that out. And yes, "Add" brings so many features and could be damaged as the behavior are more implicit rather than explicit. we should use 'COPY' instead.

Okay, and thank you for bringing this up.

hsinhoyeh · 2023-03-30T02:55:51Z

quiz/01-rate-limit/quiz01/core/migrations/0002_customer_id_model_headerfield_model.py

the idea of migration file per PR should be consolidated into one single file, not introducing multiple separated files. any particular reason for doing it in this way?

and it seems table HeaderField_Model is used nowhere.

Deleting and recreating all migrations may cause other issues, such as circular dependencies, depending on how models are constructed. If the goal is to reduce the number of files, we can also use 'Squashing migrations'. However, it should be noted that model interdependencies in Django can get very complex, and squashing may result in migrations that do not run. So if this requirement is deemed necessary or if there are too many files, please let me know again, and we will modify it using this method.

it is an open question that are we allow (or recognize) more migration files introduced into a single PR? Think about when multiple people are working on the same repo, will having multiple migrations files would simplify or complex the resolving of conflicts?

This is indeed a very common problem, and Django has provided solutions for 'Version control' and this issue.

Regarding the question of

whether to allow (or recognize) more migration files introduced into a single PR,

I don't know much about that. so in my understanding, it is similar to 'rebase' and 'merge'. If there are more complex functionalities to be uploaded, I would prefer 'merge' that has a more complete record. Of course, in general use of 'git', there are different theories on whether to use 'merge' or 'rebase' for branches too.

hsinhoyeh · 2023-03-30T02:58:22Z

quiz/01-rate-limit/quiz01/core/migrations/0003_auto_20230322_0444.py

+        migrations.CreateModel(
+            name='GET_Model',
+            fields=[
+                ('customer_ID', models.TextField(primary_key=True, serialize=False)),


add a path field, this would help you record what paths are queried.
and the rate-limited module can be used to guard method + path

Okay, it has been added.

path = models.TextField(default = 'n/a')

hsinhoyeh · 2023-03-30T03:00:13Z

quiz/01-rate-limit/quiz01/core/tests.py

+
+                # get diff header file by status code
+                if(resp.status_code == 200):
+                    print('[X-RateLimit-Limit]: '+ resp['X-RateLimit-Limit'] 


assertEqual would help you to test whether the result is expected or not
https://docs.python.org/3/library/unittest.html#unittest.TestCase.assertEqual

The main purpose of this code snippet is to observe the status of each execution of the API, rather than to make the final judgment of the result. Of course, an assertEqual has been added at the end to verify if the desired result is obtained.

hsinhoyeh · 2023-03-30T03:01:32Z

quiz/01-rate-limit/quiz01/core/tests.py

+                    + ' [X-RateLimit-Remaining]: '+ resp['X-RateLimit-Remaining'] 
+                    + ' [X-RateLimit-Reset]: '+ resp['X-RateLimit-Reset'])
+            elif(resp.status_code == 429):
+                print('[Retry-At]: '+ resp['Retry-At'] )


assertEqual can be used here again.

having print is useless when this code is running via ci/cd

Indeed, it seems that print are not necessary in the ci/cd, so they have been removed in the new version. In the previous versions, as we discussed, the code in question was used to observe the execution status, also help developers identify patterns of errors if tests failed.

def test_get_not_over(self): for i in range(1,5): resp = self.c.post('/reset/',{'group':'get', 'key':'ip', 'rate':'100/s', 'method':'GET'}) t0 = int(time.time()) for j in range(1,101): resp = self.c.get('/api/') #request api t1 = int(time.time()) if((t1-t0) <1): break if((t1-t0) >1): self.assertEqual(0, 1) #we can't get it for less than 1s else: self.assertEqual(resp.status_code, 200)```

hsinhoyeh · 2023-03-30T03:02:53Z

quiz/01-rate-limit/quiz01/Dockerfile

@@ -0,0 +1,10 @@
+# syntax=docker/dockerfile:1
+FROM python:3


FROM python:3.9

Changes have been made based on the requirements.

hsinhoyeh · 2023-03-30T03:04:38Z

quiz/01-rate-limit/quiz01/core/views.py

+        if isinstance(fn, functools.partial):
+            fn = fn.func
+
+        # Django <2.1 doesn't use a partial. This is ugly and inelegant, but


why do you want to do this?

in the docker file, you specified the Django version should be between

Django>=3.0,<4.0

why you would get a version less than 2.1?

Thank you for the reminder, it has been removed.

hsinhoyeh · 2023-03-30T03:08:41Z

quiz/01-rate-limit/quiz01/core/views.py

+
+def headerfiled_post_db(request,block_info):
+    client_id = get_client_ip_address(request)
+    if POST_Model.objects.filter(customer_ID=client_id).exists():


this feature would do what you want
https://docs.djangoproject.com/en/4.1/ref/models/querysets/#update-or-create

create or update should be done atomically inside the DB layer, having a client (e.g. Django) to do this would create a race condition when this instance is running on multiple replicas mode.

This method is excellent, and I didn't notice it before. It makes the code cleaner. Thank you for your suggestion, and I have made the changes.

def headerfiled_get_db(request,block_info): client_id = get_client_ip_address(request) GET_Model.objects.update_or_create( customer_ID = client_id, defaults=dict( path = request.path, Remaining = block_info['limit'] - block_info['count'], Reset = block_info['time_left'], RetryAt = block_info['time_left'] ) )

dirtyrabbit · 2023-04-13T08:33:35Z

All messages have been replied to. It's nice to see your response again.

root and others added 13 commits March 18, 2023 10:58

<new> Establishing the basic environment

24c5a83

<fead> DRF's Serializers & Model

039e1e0

<feat> DRF's Routers

b92df2b

<chore> Revise the Rate limiting package

2d8def3

<feat> The header field displays the rate limit judgment

bfa1797

<test> requirement test && Concurrently test

b0efae7

<feat> add sqlite datebase

7f01ce7

<refactor> view ratelimit_tracking info

957a816

<docs> create README.md && add image

2237f7a

Update README.md

34eee2b

Update README.md

c6caa76

Add files via upload

d99b5d1

Add files via upload

edfb479

hsinhoyeh requested changes Mar 24, 2023

View reviewed changes

dirtyrabbit and others added 15 commits March 24, 2023 23:11

update gitignore

ee4e702

Delete webpage.png:Zone.Identifier

f94670f

update gitgnore

f7d9866

<remove> all file from repo

b6694a6

commit file with gitignore

57ebc56

remove all file

f744ef6

<fix> commit with gitignore

a16c0fa

<remove> file under data/

4bf12a1

<fix> the count of A mistake

8671382

<feat> rate limit reset api

87cb136

<test> add execution time

8e4f150

<feat> add response function and remove tracking function

f237d4f

Update README.md

2b36e0e

<refactor> adjust test about request time of GET

fdc7270

<feat> test time greate 1s loop

f3bb7c6

dirtyrabbit added 3 commits March 28, 2023 17:26

<test> verify the response header

1703ed7

<fix> git push ignore file

5f8e886

<fix> migrations error

ba9c220

hsinhoyeh reviewed Apr 11, 2023

View reviewed changes

dirtyrabbit added 6 commits April 13, 2023 11:18

<docs> restore files

9bba224

<refactor> change dockerfile copy to add

5e24494

<refactor> modify tests print

71b31f6

<chore> python 3 to 3.9

c3fb900

<refactor> api reset

39bcb11

<feat> add path field to db

c8208e3

<refactor> change dockerfile ADD to COPY

2c4e084

quize1小作業 #1

Are you sure you want to change the base?

quize1小作業 #1

Conversation

dirtyrabbit commented Mar 22, 2023

hsinhoyeh commented Mar 24, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dirtyrabbit Mar 30, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dirtyrabbit Mar 28, 2023 • edited Loading

Choose a reason for hiding this comment

dirtyrabbit commented Mar 28, 2023

hsinhoyeh commented Mar 30, 2023

dirtyrabbit commented Mar 30, 2023

hsinhoyeh left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dirtyrabbit Apr 13, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dirtyrabbit Apr 13, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dirtyrabbit commented Apr 13, 2023

dirtyrabbit Mar 30, 2023 •

edited

Loading

dirtyrabbit Mar 28, 2023 •

edited

Loading

dirtyrabbit Apr 13, 2023 •

edited

Loading

dirtyrabbit Apr 13, 2023 •

edited

Loading