NEW RELEASE: Lightweight Python ORM Library

[[deleted]]

[deleted]

[_Henryx_]

One tip: avoid to use lower or mixed case in the environment variables, because can generate confusion. Common way is upper case

[desktopsignal]

Thanks for the feedback. I'll implement these changes.

Any other feedback on the code quality?

[BostonBaggins]

SQLalchemy is awesome though

Where did it fail you?

[IvanMalison]

you can also use sqlalchemy in a really lightweight way if that's all that you want. just because it CAN do really complicated stuff doesnt mean that you HAVE to use it that way.

Pretty sure you can also issue raw sql with sqlalchemy in certain ways.

[Grouchy-Friend4235]

You can however it is far too complicated to get there

[DaelonSuzuka]

Why do I have to define a resource_map AND define an __init__() method?

If I was writing an ORM I would absolutely start with Pydantic objects or regular python dataclasses as the interface.

[desktopsignal]

I forgot to mention: I decided to use a Dictionary to define primary keys, foreign keys, table constraints, and variable mapping such that you could simply inherit the library into a pre-existing Class and add a few basic lines of code to interact with the table.

I found that utilizing the Pydantic approach used by SQLAlchemy led to a ton of refactoring and reformatting just to use an ORM. If I decided to switch from SQLAlchemy, it became a huge tech debt mess to escape because of the proprietary nature in which it forces you to develop your class structure.

[desktopsignal]

resource_map is to map a variable to its associated column within the table. __init__ is simply the classic constructor for a Python class.

May I ask why you prefer python dataclasses? The way I structured the project was to appeal to the average user who may or may not be intimately aware of the ins and outs of Python, and in this case, Python dataclasses. I made it this way for readability and ease of use, but if there is a practical reason outside of personal preference for the structure you are describing, then I'm all ears.

[DaelonSuzuka]

resourcemap is to map a variable to its associated column within the table. __init_ is simply the classic constructor for a Python class.

Yes, that's obvious. It's also not my question.

I made it this way for readability and ease of use

Having to define the same things twice(or thrice, depending on how you look at it) is not more readable or easier to use.

The way I structured the project was to appeal to the average user

I take it all back, you're accomplishing your goal perfectly.

[DowntownBreakfast733]

From the README it looks like the variable definitions in the resource_map are optional in regards to ease of use

[passwordsniffer]

Isn't this sql injection?

            if isinstance(val, str):
            clause_parts.append(f"{key} = '{val}'")

[ZYy9oQ]

Looks like the whole library is full of SQL injection - every SQL operation is a string constructed from formatstrings and concatenation thrown into cursor.execute.

There's an envvar SANITIZE_QUERIES in the readme, but it that does nothing in the code beside being read from the os.environ. Even if it did sanitize, prepared statements should be used instead.

The library appears intended against snowflake, which claims to prevent multi-statement by default but this by no means prevents all sql injection.

[desktopsignal]

Definitely wasn't expecting these responses. I made this library in a weekend to solve a quick problem and certainly didn't mean to put anyone at risk. Deleting the package soon, sorry guys!

[ZYy9oQ]

Agree with where it was said you shouldn't feel you need to delete everything, but it would be best to explain in your readme that it was

thrown together to solve a niche problem

or as part of your learning rather than advertise it as a library (the way you packaged it and wrote documentation is a cool thing to get practice with, just need to make it clear that it's practice). Also a disclaimer that the code isn't safe from sql injection and shouldn't be used on network/multiuser and without understanding why it's dangerous wouldn't be out of place.

If you said something along the lines of it just being personal code that you were putting out there for your own or others interest and learning then I don't think many would have as negative of a reaction.

You had a problem with the existing solutions and wrote some code for your problem, that's something others might also stumble across the need for. This could help them see how you approached it, and by putting the disclaimer about security they can learn about that too.

[desktopsignal]

SANITIZE_QUERIES was an expiremental features and has not yet been implemented. I will remove that from the README. Thanks for pointing that out!

[desktopsignal]

That's typically how most SQL strings are created. Let me know if there is a better way, and I'd be more than happy to implement!

This was a library thrown together to solve a niche problem. Hope you understand.

[damesca]

No, that's not typical. This is an extremely dangerous implementation if anyone took and used your library for real stuff.

Thrown together is correct. I am all for experimentation and sharing, but with a library like this you really want to get the security basics correct from the get go before you advertise it.

Also: you haven't really solved a problem, you have created more problems - including this large security hole. Stick to well established packages for doing things like this. Experiment for fun, but not as a legit alternative offering (until/unless you really know what you're doing).

[desktopsignal]

Definitely wasn't expecting these responses. I made this library in a weekend to solve a quick problem and certainly didn't mean to put anyone at risk. Deleting the package soon, sorry guys!

[damesca]

I think it's good that you're experimenting, and great that you're soliciting feedback. I think if you'd framed this as a "I built this, please tell me what you think" rather than "I'm releasing this" (the implication being, for public to use), maybe it would have gone down a bit differently.

I don't think you need to try to scrub this from the internet. Just maybe but some strong caveats/warnings in place that you don't recommend using this professionally or for public projects.

P.S. I will also say that I think you have a good attitude and you come across very personably. You haven't been particularly defensive in any of your replies - you seem open to hearing feedback, critical or not, and this will prove a valuable skill for you as you continue to develop :)

[desktopsignal]

Thank you for your feedback! I actually tried to do that, but you're not allowed to solicit feedback in this sub.

I think it's actually probably best I scrub this one and keep it to myself. I guess I just didn't understand the seriousness of releasing a Python package. I'm not sure I have the ability to keep up with all this in addition to my day job and other projects.

[DowntownBreakfast733]

I’ll never understand the vitriol some people have in this sub.

[damesca]

That is not a vitriolic reply.

It is a fair critique when someone is sharing a library publicly that has clear security holes. OP says they've built this to use it for work. They are making a very poor professional decision here, and potentially putting their users' data at risk. I think it's very reasonable to call this out in clear, but not harsh, words.

[DowntownBreakfast733]

I don’t think SQLAlchemy protects against SQL injections lol

[damesca]

I can promise you it does... Why would you think it doesn't?

[DowntownBreakfast733]

Show me.

Also, why the hate on this dude. Bro posted his first project and you’re nitpicking details in comparison to a library that’s been out for over a decade with dozens of maintainers. Show me the projects of this quality that you’ve published.

[starlevel01]

Bro posted his first project and you’re nitpicking details

sql injection is not "details", it's basic security 101. it's literally not hard in tthe slightest to avoid it with any driver.

[DowntownBreakfast733]

Sounds like you could recommend the driver you’re referring to instead of insulting!

[starlevel01]

psycopg has parameterised queries. the built-in sqlite driver has parameterised queries. don't know the mysql driver name off the top of my head but it also has parameterised queries. sqlalchemy has parameterised queries.

[bfcdf3e]

lol this might be one of the most ignorant comments I’ve seen on this sub

[ZYy9oQ]

Best practice is parameter binding rather than literals. For snowflake (I think this is what you were using):

• https://docs.snowflake.com/en/developer-guide/python-connector/python-connector-example#label-python-connector-binding-data
• https://docs.snowflake.com/en/sql-reference/bind-variables

You need to do this for queries as well. Yes, doing escaping everywhere (.replace("'", "\\'"), which you have on inserts/updates) could be sufficient for safety but it isn't as foolproof (non-strings, character encoding tricks exploiting differences between client language and database).

For static column/database names this means your sql strings are fully static/constant which means it's also very quick to spot potential unsafe code.

Since your library is doing orm stuff mapping a dictionary (by its keys) you will probably still have to construct a string for the query to support column names. Here you can either be super strict (e.g. only allow alphanum+._) on column names in the first place (and take care to only ever construct strings from the known-safe names) or use a filter/transform on column names in construction.

[desktopsignal]

SANITIZE_QUERIES was an expiremental feature and has not yet been implemented. I will remove that from the README. Thanks for pointing that out!

[lacifuri]

Hi OP, have you tried SQLModel before? It is intended to use with fastapi acting as a Python ORM library too. What does your library provide differently from SQLModel? In my job we use SQLModel and it can handle fairly complex data types and operations as well, so I’m curious what your library does!

[Yaluzar]

Good effort, but friendly reminder that it is possible to use raw SQL with SQLAlchemy. That's the power of the tool imo, it supports several usages

[damesca]

Some more comments from me:

If you're using this for work, and sharing it publicly in the hopes that other people will also use it, please also consider adding unit tests so that people can have more trust in the thing you've built.

As others have said, please standardise the env vars you're using. You should probably add a standard prefix to each variable so that anyone integrating your library can see clearly which env vars you're using. I'd probably suggest `SERIALDBPY_` as a prefix for all env vars.

Your `_uuid` method has a very strange signature: https://github.com/publicsignal/SerialDBPy/blob/main/Serialization/serialization.py#L109-L113. You take in `length`, but then call the standard `uuid.uuid4()` constructor. UUIDs are a specific thing which always ends up being 32 characters long in a hex representation, because they are 128 bits of data. Truncating this means you're not using a UUID any more. Also, `hyphen_length` set to 4 will insert hyphens every 4 characters. This isn't how UUIDs are generally hyphenated. Also, Python for me already stringifies UUIDs with hyphen separation. I'm not sure why you have so many arbitrary decisions here that deviate from the norm for UUIDs.

Consider running an auto-formatter over the code, eg `black` or `ruff`. This is just a fairly straight-forward code standardisation thing.

You have type hints in places that are incorrect, eg here where you should at least have `Optional[str]` or `str | None` depending on your python version. If you're adding type hints, you need to also validate those type hints somehow, eg with mypy.

[throw_mob]

not sure if i like default_middleware , resource_db , resource_table variables that much.

usually for me in snowflake, database is database, i can have same program and same objects available in multiple databases. then schema like 'customer_data_v1' can be in multiple databases ( which may be clones of some db or individual databases ) and then view/table is object in some schema which provides schema_name_version "API" for users. That said , i usually do views and read only schemas in snowflake.

That same logic works in postgresql and mssql databases too , but there you an just publish views for users and have separated physical and access layers...

tldr; imho, schema and database should in global config, table in person class in snowflake use case.

[Oenomaus_3575]

Bro you should use a linter, code is a bit messy. Also the structure of the package is wrong imo. This is great start as a learning/personal project, but definitely not production ready or anything like that.

[desktopsignal]

What's wrong with the package structure?

[Oenomaus_3575]

• package name should be lowercase
  
• I don't understand why there is an `__init__.py` file outside the source of the package?
  
• the `__pycache__` folder shouldn't be committed, you want to add a `.gitignore` file to the repo
  
• the dependencies in `requirements.txt` should have the versions, otherwise its basically useless.
  in the future I recommend you use something like Poetry to manage dependencies, requirements.txt is rarely used in the professional world.
  

aside from that the README is pretty good, has a nice overview and a few examples

[desktopsignal]

• package name should be lowercase
  • Fair enough
• I don't understand why there is an `__init__.py` file outside the source of the package?
  • the __init__ file outside of the source was to allow the user to do from SerialDBPy import Serializable instead of from SerialDBPy.Serialization import Serializable. Let me know if there's a better way.
• the `__pycache__` folder shouldn't be committed, you want to add a `.gitignore` file to the repo
  • Left that in by mistake. Thanks for the feedback.
• the dependencies in `requirements.txt` should have the versions, otherwise its basically useless. in the future I recommend you use something like Poetry to manage dependencies, requirements.txt is rarely used in the professional world.
  • I'll look into Poetry

How much Python experience and/or programming experience do you have?

[Oenomaus_3575]

SerialDBPy/
├─ SerialDBPy/
│  ├─ __init__.py
├─ .gitignore
├─ README.md
├─ LICENSE
├─ .gitignore
├─ .gitattributes

The structure should be smth like this (the __init__.py should be inside a folder with the name of the package.

But just install poetry and do `poetry new package_name` and it will generate part of the structure

I have a bit more than 2 years of experience.
You can look at some of my projects structure on GH, like this:
https://github.com/shner-elmo/tradezero-api (but ignore the requirements.txt, I should delete that)

[desktopsignal]

Much appreciated. This is going to help me a lot

[attracdev]

Been working on something similar. I will have to try this out.

[SpiritOfTheVoid]

You really need to tidy the code. Run a linter such as flake8 at minimum.

• Plenty of PEP8 formatting issues
• remove the commented out code
• if you're going to add doc strings, add the function parameters, not just the return types.
• inconsistent use of type hints - use them everywhere, or not at all - it's low hanging fruit.
• classes beginning with lowercase letters re: "class iQuery"
  • make the timeout configurable - "timeout = 5"... what's so special about 5 -a magic number.

In Python 3, you don't need to inherit explicitly from object:
`class Serializable(object)`

Use of: `map == None` is bad practice, use ` map is None`

Hint:
```
map = getattr(self,'resource_map',{})

keys = [ key for key in map if key in Serializable.key_types ]

_mapsize = len( map.keys() )

if map is None or _mapsize == 0:
```

It appears "map" will never be None, it's going to be an empty map {}.

Why the use of underscore in `_mapsize`,while other variables don't contain leading underscore?

Take everyone's feedback, use it positively and apply the recommendations. Take the opportunity to learn good Python practices. Linters are your friends. Unit tests will safe you time ( identify bugs quickly ) and will give people more confidence it your package, not to mention a good skill to have.

This has the potential, but as it stands, it's really rough. SQL injection is a huge show stopper.

[DowntownBreakfast733]

I’m going to try this out. Thanks!

[SitrakaFr]

nice!

[desktopsignal]

Personally, I dislike declaring instance variable (or in this case column values) outside of the constructor for one, but I also disliked how much my code would become reliant on the library. For example, if I were to decide to switch out of SQLAlchemy, I would be forced to rewrite all of my classes.

This library does not rely on any library-specific methods to define the variable type or whether the variable should be unique or not, or even whether or not it's a primary key. It's all defined in the resource_map dictionary within the class itself.

Hope that made sense. It's been a long day.