The SchoolClassModel in the Kelvin API exits when a class name contains one character. For example, "3". But this should be ok in UCS@school as a class name, because the class name in UCS@school is always preceded by the OU. For example "SCHULEXY-3". So the validation should actually check "school-name". So probably the string must be reassembled at this point. Here the traceback: 2020-05-27 09:57:48 ERROR Exception in ASGI application Traceback (most recent call last): File "/kelvin/venv/lib/python3.7/site-packages/uvicorn/protocols/http/httptools_impl.py", line 385, in run_asgi result = await app(self.scope, self.receive, self.send) File "/kelvin/venv/lib/python3.7/site-packages/uvicorn/middleware/proxy_headers.py", line 45, in __call__ return await self.app(scope, receive, send) File "/kelvin/venv/lib/python3.7/site-packages/fastapi/applications.py", line 140, in __call__ await super().__call__(scope, receive, send) File "/kelvin/venv/lib/python3.7/site-packages/starlette/applications.py", line 134, in __call__ await self.error_middleware(scope, receive, send) File "/kelvin/venv/lib/python3.7/site-packages/starlette/middleware/errors.py", line 178, in __call__ raise exc from None File "/kelvin/venv/lib/python3.7/site-packages/starlette/middleware/errors.py", line 156, in __call__ await self.app(scope, receive, _send) File "/kelvin/venv/lib/python3.7/site-packages/starlette/exceptions.py", line 73, in __call__ raise exc from None File "/kelvin/venv/lib/python3.7/site-packages/starlette/exceptions.py", line 62, in __call__ await self.app(scope, receive, sender) File "/kelvin/venv/lib/python3.7/site-packages/starlette/routing.py", line 590, in __call__ await route(scope, receive, send) File "/kelvin/venv/lib/python3.7/site-packages/starlette/routing.py", line 208, in __call__ await self.app(scope, receive, send) File "/kelvin/venv/lib/python3.7/site-packages/starlette/routing.py", line 41, in app response = await func(request) File "/kelvin/venv/lib/python3.7/site-packages/fastapi/routing.py", line 127, in app raw_response = await dependant.call(**values) File "/kelvin/kelvin-api/ucsschool/kelvin/routers/school_class.py", line 148, in search return [await SchoolClassModel.from_lib_model(sc, request, udm) for sc in scs] File "/kelvin/kelvin-api/ucsschool/kelvin/routers/school_class.py", line 148, in <listcomp> return [await SchoolClassModel.from_lib_model(sc, request, udm) for sc in scs] File "/kelvin/kelvin-api/ucsschool/kelvin/routers/base.py", line 99, in from_lib_model return cls(**kwargs) File "/kelvin/venv/lib/python3.7/site-packages/pydantic/main.py", line 283, in __init__ raise validation_error pydantic.error_wrappers.ValidationError: 1 validation error for SchoolClassModel name Value must not contain anything other than digits, letters, dots or spaces, must be at least 2 characters long, and start and end with a digit or letter! (type=value_error)
*Untested* patch available in git branch dtroeder/51363_school_class_name_validation. [dtroeder/51363_school_class_name_validation e58a71dc6] Bug #51363: validate 'OU-name' instead of 'name' for school classes
(In reply to Daniel Tröder from comment #1) > *Untested* patch available in git branch > dtroeder/51363_school_class_name_validation. > > [dtroeder/51363_school_class_name_validation e58a71dc6] Bug #51363: validate > 'OU-name' instead of 'name' for school classes Failed with: 2020-06-01 23:32:53 ERROR Exception in ASGI application Traceback (most recent call last): File "/kelvin/venv/lib/python3.7/site-packages/uvicorn/protocols/http/httptools_impl.py", line 385, in run_asgi result = await app(self.scope, self.receive, self.send) File "/kelvin/venv/lib/python3.7/site-packages/uvicorn/middleware/proxy_headers.py", line 45, in __call__ return await self.app(scope, receive, send) File "/kelvin/venv/lib/python3.7/site-packages/fastapi/applications.py", line 140, in __call__ await super().__call__(scope, receive, send) File "/kelvin/venv/lib/python3.7/site-packages/starlette/applications.py", line 134, in __call__ await self.error_middleware(scope, receive, send) File "/kelvin/venv/lib/python3.7/site-packages/starlette/middleware/errors.py", line 178, in __call__ raise exc from None File "/kelvin/venv/lib/python3.7/site-packages/starlette/middleware/errors.py", line 156, in __call__ await self.app(scope, receive, _send) File "/kelvin/venv/lib/python3.7/site-packages/starlette/exceptions.py", line 73, in __call__ raise exc from None File "/kelvin/venv/lib/python3.7/site-packages/starlette/exceptions.py", line 62, in __call__ await self.app(scope, receive, sender) File "/kelvin/venv/lib/python3.7/site-packages/starlette/routing.py", line 590, in __call__ await route(scope, receive, send) File "/kelvin/venv/lib/python3.7/site-packages/starlette/routing.py", line 208, in __call__ await self.app(scope, receive, send) File "/kelvin/venv/lib/python3.7/site-packages/starlette/routing.py", line 41, in app response = await func(request) File "/kelvin/venv/lib/python3.7/site-packages/fastapi/routing.py", line 127, in app raw_response = await dependant.call(**values) File "/kelvin/kelvin-api/ucsschool/kelvin/routers/school_class.py", line 161, in search return [await SchoolClassModel.from_lib_model(sc, request, udm) for sc in scs] File "/kelvin/kelvin-api/ucsschool/kelvin/routers/school_class.py", line 161, in <listcomp> return [await SchoolClassModel.from_lib_model(sc, request, udm) for sc in scs] File "/kelvin/kelvin-api/ucsschool/kelvin/routers/base.py", line 99, in from_lib_model return cls(**kwargs) File "/kelvin/venv/lib/python3.7/site-packages/pydantic/main.py", line 281, in __init__ values, fields_set, validation_error = validate_model(__pydantic_self__.__class__, data) File "/kelvin/venv/lib/python3.7/site-packages/pydantic/main.py", line 849, in validate_model v_, errors_ = field.validate(value, values, loc=field.alias, cls=cls_) File "/kelvin/venv/lib/python3.7/site-packages/pydantic/fields.py", line 523, in validate v, errors = self._apply_validators(v, values, loc, cls, self.post_validators) File "/kelvin/venv/lib/python3.7/site-packages/pydantic/fields.py", line 671, in _apply_validators v = validator(cls, v, values, self, self.model_config) File "/kelvin/venv/lib/python3.7/site-packages/pydantic/class_validators.py", line 276, in <lambda> return lambda cls, v, values, field, config: validator(cls, v, values=values, field=field, config=config) File "/kelvin/kelvin-api/ucsschool/kelvin/routers/school_class.py", line 66, in check_name class_name = f"{values['school']}-{values['name']}" KeyError: 'school'
The school name is only available via the ucsschoolRole property at this point. The altered code takes this into account. A unit-test checks classes with names including characters a-z0-9 of varying length (1-32) can be accessed via get from the kelvin api. [dtroeder/51363_school_class_name_validation] 2b911820c Bug #51363: Correct patch and new unit-test
Looks good so far. A thing I would change to make the code more future proof: In the roles.py of the ucsschool-lib exists a function 'get_role_info' that implements almost exactly your logic for extracting the school name. Please use it (take care to handle the possible Exception). Also even if currently the case, we do not know exactly what custom roles might be attached to an object and which roles we might add to school classes in the future. I would not just use the first existing role, but iterate over all roles and take the first one with context_type==school & role==school_class
[dtroeder/51363_school_class_name_validation] d3c97950b Bug #51363: unit-test [dtroeder/51363_school_class_name_validation] 98cacecd2 Bug #51363: root validator Thanks for the review! Now I used the root_validator, which anticipates the whole object including the (class-) name and school. Since the validator of the derived class UcsSchoolBaseModel is called afterwards, I added a 'pass-through' validator. Both methods are added for SchoolClassPatchDocument, too. Unfortunately, root_validator cannot be reused as the validator, so I had to copy that code. I also added a simple unit-test- the test before was actually an integration test.
Patching a school_class does not work anymore: 2020-05-18 07:53:07 INFO 172.17.42.1:54320 - "PATCH /ucsschool/kelvin/v1/classes/DEMOSCHOOL/9 HTTP/1.1" 500 2020-05-18 07:53:07 ERROR Exception in ASGI application Traceback (most recent call last): File "/kelvin/venv/lib/python3.7/site-packages/uvicorn/protocols/http/httptools_impl.py", line 385, in run_asgi result = await app(self.scope, self.receive, self.send) File "/kelvin/venv/lib/python3.7/site-packages/uvicorn/middleware/proxy_headers.py", line 45, in __call__ return await self.app(scope, receive, send) File "/kelvin/venv/lib/python3.7/site-packages/fastapi/applications.py", line 140, in __call__ await super().__call__(scope, receive, send) File "/kelvin/venv/lib/python3.7/site-packages/starlette/applications.py", line 134, in __call__ await self.error_middleware(scope, receive, send) File "/kelvin/venv/lib/python3.7/site-packages/starlette/middleware/errors.py", line 178, in __call__ raise exc from None File "/kelvin/venv/lib/python3.7/site-packages/starlette/middleware/errors.py", line 156, in __call__ await self.app(scope, receive, _send) File "/kelvin/venv/lib/python3.7/site-packages/starlette/exceptions.py", line 73, in __call__ raise exc from None File "/kelvin/venv/lib/python3.7/site-packages/starlette/exceptions.py", line 62, in __call__ await self.app(scope, receive, sender) File "/kelvin/venv/lib/python3.7/site-packages/starlette/routing.py", line 590, in __call__ await route(scope, receive, send) File "/kelvin/venv/lib/python3.7/site-packages/starlette/routing.py", line 208, in __call__ await self.app(scope, receive, send) File "/kelvin/venv/lib/python3.7/site-packages/starlette/routing.py", line 41, in app response = await func(request) File "/kelvin/venv/lib/python3.7/site-packages/fastapi/routing.py", line 119, in app dependency_overrides_provider=dependency_overrides_provider, File "/kelvin/venv/lib/python3.7/site-packages/fastapi/dependencies/utils.py", line 548, in solve_dependencies required_params=dependant.body_params, received_body=body File "/kelvin/venv/lib/python3.7/site-packages/fastapi/dependencies/utils.py", line 678, in request_body_to_args v_, errors_ = field.validate(value, values, loc=("body", field.alias)) File "/kelvin/venv/lib/python3.7/site-packages/pydantic/fields.py", line 509, in validate v, errors = self._validate_singleton(v, values, loc, cls) File "/kelvin/venv/lib/python3.7/site-packages/pydantic/fields.py", line 664, in _validate_singleton return self._apply_validators(v, values, loc, cls, self.validators) File "/kelvin/venv/lib/python3.7/site-packages/pydantic/fields.py", line 671, in _apply_validators v = validator(cls, v, values, self, self.model_config) File "/kelvin/venv/lib/python3.7/site-packages/pydantic/class_validators.py", line 311, in <lambda> return lambda cls, v, values, field, config: validator(v) File "/kelvin/venv/lib/python3.7/site-packages/pydantic/main.py", line 544, in validate return cls(**value) File "/kelvin/venv/lib/python3.7/site-packages/pydantic/main.py", line 281, in __init__ values, fields_set, validation_error = validate_model(__pydantic_self__.__class__, data) File "/kelvin/venv/lib/python3.7/site-packages/pydantic/main.py", line 875, in validate_model values = validator(cls_, values) File "/kelvin/kelvin-api/ucsschool/kelvin/routers/school_class.py", line 126, in check_name2 school = values['school'].split('/')[-1] KeyError: 'school' This is due to the fact, that the SchoolClassPatchModel does not have a school field and thus cannot use it in validation. Also cls.Config.lib_class.name.validate(class_name) cannot work since the Patch-Document does not have an inner Config class.
Created attachment 10385 [details] Patch for validation of patch
My proposal for now would be to just remove validation for the name from PatchDocument. If a validation before calling lib_obj.modify() is desired I would it like in the attached patch
[dtroeder/51363_school_class_name_validation] c0ad7069c Bug #51363: Applied patch and remove validation from PatchDocument Good suggestion. I applied the patch and removed the validation from SchoolClassPatchDocument.
[dtroeder/51363_school_class_name_validation] 0f1305e8b Bug #51363: revert last commit and check against dummy-school As discussed, I reverted the last commit and validate against `DEMOSCHOOL-name` in PatchDocument, since we know the school has a valid name at this point.
[dtroeder/51363_school_class_name_validation] 422efc445 Bug #51363: fix integration test The integration test did create classes with the name '1a' (via udm) instead of 'DEMOSCHOOL-1a'. I corrected this. The kelvin-api still returns '1a' as class-name as expected.
Okay now everything looks good to me. REOPENED for merge into feature/kelvin
RESOLVED: Merged into feature/kelvin. Version is v1.1.1 [feature/kelvin] 12e125d44 Bug #51363: add changelog entry [feature/kelvin] f7b327c17 Bug #51363: include SchoolClassPatchDocument validation [feature/kelvin] cd0883a1d Bug #51363: unit-test [feature/kelvin] e9159d5c5 Bug #51363: Correct patch and new integration-test [feature/kelvin] e58a71dc6 Bug #51363: validate 'OU-name' instead of 'name' for school classes Comment: I squashed and renamed multiple commits before merging, no code changes were made.
Merge: OK new Test: OK Class names can now be only one character long and start with numbers since the prefix $SCHOOLNAME- is now taken into consideration during validation.
Code was released with app version "1.1.1".